-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump tutor requirements to allow v17 #66
Conversation
FYI @fghaas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me; I just have two nits that I'd ask you to fix.
Do I assume correctly that you have already tested this, on Quince?
CHANGELOG.md
Outdated
@@ -1,3 +1,7 @@ | |||
## Unreleased | |||
|
|||
* [Enhancement] Support Tutor 17 and Open edX Quince |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add full stop (.
) at the end please. :)
Let me check on the build failures from |
@gabor-boros #67 is now merged; when you update your topic branch please be sure to rebase it on current |
@fghaas Thanks for the review. I'm going to address the changes soon. The quince compatibility was not yet tested however the new version of tutor should not break this plugin as far as I know. Though I'll do a test round. Converted the PR to draft to indicate its status better. |
796b176
to
549db06
Compare
Hey @fghaas. We are currently blocked on overhangio/tutor#958 |
@fghaas The changes are tested, the plugin is working and the upstream issue is resolved as well. This is ready to be reviewed and merged. |
@gabor-boros Hold on, I don't quite follow. If this plugin broke on Quince as long as overhangio/tutor#958 wasn't merged, then that means it would break on 17.0.0. So, don't we have to wait until a 17.0.1 release to get the dependencies right? |
Sorry for the confusion. Let me clarify: The plugin was never broken, but we were not able to test it as the nightly branch of Tutor was broken. This plugin perfectly works on Quince, I just tested it today. |
Ah, gotcha! Sounds good to me then. Can I bother you for one more amend and force-push, so we get a meaningful commit message body? Something like
I would much appreciate that. Thanks! |
549db06
to
1655f84
Compare
Update the install_requires list to include support for Tutor 17 (and Open edX Quince), and update the README in anticipation of a 1.3.0 release. Signed-off-by: Gabor Boros <[email protected]>
1655f84
to
b1900d9
Compare
@fghaas how about now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. Thanks a lot!
@gabor-boros Could you do me a huge favour and have a brief look into #70? I am a bit surprised that you didn't run into that during your tests, and would like to find out why not. |
@fghaas I'm surprised as well, though there were some dependency caching issues on our end around the time of this PR, so probably the dependencies were just simply not using the new package version. Though this change is 2 months old, so I cannot recall exactly. |
Description
This PR bumps Tutor to v17 to support Quince. Also, it updates the changelog and the compatibility matrix.