Let a Studio user export course to git (and via git, to elsewhere, eg LMS) #718
Conversation
|
Hi Ike, Thanks for the PR. There's a lot to digest here. I'll be looking at this over the weekend. |
scripts/cms_export_to_github
Outdated
There was a problem hiding this comment.
Is there a reason why this needs to be a separate script. Can't this just be in push_to_lms.py?
There was a problem hiding this comment.
It's in a separate script because so much of the script is system dependent - filesystem locations, github protocol. good to be able to edit it without having to restart the CMS. We also run it by hand or from external scripts occasionally.
|
What's the status of this pull request? |
|
Why did you close this? This PR is actively in use at MIT and we want it merged into edX-platform. |
|
Ike, Dave closed the pull request because he asked about the status and got no response for three weeks. We assume that the contributor will get notifications when comments are made on the pull requests. Is that not the case here? We need to have a better understanding of how these pull requests will flow into edx-master. I'm concerned about this pull request: it has no tests. It's not possible to create and maintain a quality code base the size of edx-platform without tests: even if the code works well now at MIT, it will be impossible to keep it working properly in the face of other changes without tests that verify it is working properly. We want edx-platform to be successful. It won't be if we have untested code. |
cms/templates/widgets/header.html
Outdated
There was a problem hiding this comment.
Can you fix the whitespace here? It looks like it's mixing tabs and spaces.
|
More generally, I think this feature would be great to have as a proper Studio feature, particularly if we could set it up so that users could push to arbitrary github repositories as part of a more general export workflow. I suspect that that request doesn't fall into the purview of this particular PR, though. |
|
I've gone ahead and done a pretty major rework on this. It should be suitably generalized, I added support for https based repos, support for switching the remote repo url, has good test coverage, updated UI to match other tool pages, and I tried to address all the comments. I'd be interested in hearing what you envision as to how it might integrate into a more generalized export. Maybe some future work to add it as an export option to the existing tar ball export and prompt for github credentials, or something else? I'd definitely be interested in helping to make that happen as we are obviously pretty vested in git based course development. |
There was a problem hiding this comment.
@nedbat: Can you weigh in on these return codes, and perhaps suggest a cleaner way of handling things (similar things are happening here: https://github.com/edx/edx-platform/pull/1942/files#diff-03fed087942ff12728ffd75feb7f666eR27, with rationale here: https://github.com/edx/edx-platform/pull/654/files#diff-03fed087942ff12728ffd75feb7f666eR52)
There was a problem hiding this comment.
The global structure of this code is odd: since you have a function (export_to_git) that you want to use from a few places, put it someplace central, then import it where you need it. Importing from a management command into a view is confusing.
There was a problem hiding this comment.
I'm opposed to using return codes, and don't find the reasoning about multiple uses compelling. At the very least, create one exception class and use it to carry a reason code. None of these return codes are tested explicitly, except in the tests. If you create an exception, you can carry an actual readable message, which will be simpler and easier to understand than the logging monkey-patching that's happening in the view function now.
There was a problem hiding this comment.
Sounds good. I knew it wasn't terribly elegant, and really just put it in place to help with testing.
There was a problem hiding this comment.
I should also ask, where is the preferred spot to put this, utils.py, push_to_lms.py or a new file? The organization was largely based on them previously being shell scripts, and I hadn't put much thought into where it should go, but I agree with it going somewhere other than the command.
There was a problem hiding this comment.
If we can make this code its own app, then you can create a module in that app to hold the export_to_git function.
There was a problem hiding this comment.
Would we then also want to move the export and import views out to their own app? It just seems this is pretty tightly related to those functions, as eventually we may want to be able to export and import formats other than xml as well. Should I maybe just add the view to the existing import_export.py and put the "export_to_git" function in there as well, or maybe move that function into the modulestore lib somewhere around export_to_xml?
|
@carsongee can you rebase and resubmit in order to get some real test results? Apparently there was a bug introduced late last week which cased generic widespread test failure. It's been since repaired. |
There was a problem hiding this comment.
startswith accepts a tuple of possibilities, so you can shorten these last three lines to: not repo.startswith(('http:', 'https:', 'file:'))
|
The tests got ran with the fresh rebase and the ones that failed are passing locally. Any guesses on what may be different? I'm running off the devstack vagrant box locally. It appears to be failing on push, which usually means the bare repo wasn't properly deleted (no changes to push), but that should be happening on each test with tearDown. |
|
This turned out to be some git environment variables that were set on the Jenkins server that overrides local .git/config values. I added a lengthy skip conditional to overcome that which explains some of the missing test coverage. It also appears the Jenkins server's having what appears to be either an older or newer version of git than the devstack image which was also causing errors. |
|
@carsongee Let's talk about where we stand on this pull request. |
|
I have refactored this to be "Export to Git" and moved the common methods to their own utils file. As far as I know this is all that needed to be done thus far. |
There was a problem hiding this comment.
I'm a little uncomfortable putting a default here. I think it would be far safer to let this fail if the setting hasn't been set properly.
There was a problem hiding this comment.
Sounds good. I'll move that out.
|
I think those are my only comments. @nedbat ? |
|
Hopefully I've addressed all your comments @dianakhuang Let me know if anything needs more work from your perspective, and thank you for the review! |
CHANGELOG.rst
Outdated
There was a problem hiding this comment.
Not sure why this line got joined together.
|
@carsongee I've reviewed everything. A few minor nits, some tests to fix, and a new understanding (on our part) of some i18n things. We are very close to being done! Thanks for being patient with us. |
Added tests Converted scripts to django commands Removed advanced module requirement Generalized to platform Switched to subprocess for shell commands Beefed up resiliency and error checking. Refactored since #1910 removed get_location_and_verify_access Added settings to aws for export directory and reworked test setup and teardown Several review based fixes Added line to Changelog Changed URL handler to be accepting and moved git bare repo inside of test_root/data Added exception logging to help trace issues Added output in exception logging Made the branch to commit to explicit instead of implicit Skipping git identity test on condition of global configuration set
All forward facing wording changed to Export to Git Export to git functions removed from management command and put in common file Additional error checking and documentation improvements Nitpicks and other minor fixes
|
This is all cleaned up now. |
|
@carsongee thanks for all the work. Merge it! 👍 |
Let a Studio user export course to git (and via git, to elsewhere, eg LMS)
…op-children Feature/cale/cms drag and drop children
bump organizations app version
…eport Alter CME data report according to new requirements.
…nds (#718) - Pause and warn when `dev.pull` and `dev.up` are run without a service specified - Introduce a `*.large-and-slow` variation for when using the default large set is intentional We've seen that a lot of developers use `dev.pull` and `dev.up` and then experience the resulting pain around bandwidth and memory. This is an experiment in education -- rather than responding in chat when someone asks where all the RAM has gone, can we guide people away from these commands in a tighter loop? (Previously merged as commit 80ed748/PR #708; this contains some improvements on that.) See ADR for additional information. Implementation notes: - Keeping the main text out of the cowsay allows us to have variable length text (interpolate the make target) without messing up the speech bubble borders or having to install the cowsay package. - If a Makefile has `dev.X.default: dev.X.$(DEFAULT_SERVICES)` with no statements followed by `dev.X.%: ...` then make would end up running `dev.X.%` for *both* the DEFAULT_SERVICES and `default`, and the latter would error out. The fix here is just to ensure that there is at least one statement, even if it does nothing. (Using a different pattern such as `default_dev.X` would also have worked but been confusing, and this solution is conducive to a small explanatory comment.) Changes from previous PR: - Makefile bugfix as mentioned above. - Just include `pull` and `up` for now; `provision` and `check`, are good candidates for after this is proved out. (`migrate` and `reset` can be included later as well.) - Remove "deprecated" comment from old default targets. We are not actually deprecating them at this point, just warning people about inadvertent use of them. - Use name `large-and-slow` instead of `default`; the contents of `DEFAULT_SERVICES` shouldn't *be* the default, and are really more of an 80%-case that covers most people's workflows (but includes too much.) Rename warning script accordingly. - Update README section "Service List" with better instructions and give a pointer from the setup instructions, including some service combinations. - Update other doc and Makefile locations to reference either the "big hammer" or the "small hammer" as appropriate to the context. - Don't print directory "changes" on this recursive make. - Use `lms` as example service in the warning message, since it's a common case. - Specify how to cancel the command (same key combo works on both Mac and Linux). - Add ADR.
One of the challenges of using Studio is that no history of changes is recorded. Also, switching content between "public" and "private" leads to an inconsistent student experience; while an author is editing content, it must be "private" and thus inaccessible to students. Students may thus see modules disappear from the Edge server, while it is being edited.
Instead of using Edge, at MIT we are exporting content from Studio, and importing this into an LMS (with mongo modulestore). Studio users are presented with a "Push to LMS" page, via which they can initiate an export from Studio to github. The export goes to the
giturlwith which the course is configured. The github instance can then be configured with a web hook to trigger the LMS to pull the repo and update.Studio authors only see this option if the string
"allow_push_to_lms"is in theadvanced_modulessettings list.This workflow has the added advantage that a history of student-facing changes is recorded in github each time content is pushed to the LMS from Studio.
This feature is enabled by
MITX_FEATURES['ENABLE_PUSH_TO_LMS']=True