Course import on site creation

[melvinsoft]

This part of the trial revamp objective of Q1. I reworked Filip's old function.

The PR introduces the following new settings:

• IMPORT_TAHOE_DEFAULT_COURSE_ON_SITE_CREATION this goes inside FEATURES dict, it's a boolean to activate the course import or not.
• TAHOE_DEFAULT_COURSE_NAME: The name of the course.
• TAHOE_DEFAULT_COURSE_GITHUB_ORG: The org in Github.
• TAHOE_DEFAULT_COURSE_GITHUB_NAME: The course name in Github (as it goes in the URL).
• TAHOE_DEFAULT_COURSE_VERSION: The version or release of the course.

The new course import function is triggered in a celery task when the site is being created, pulls the course from github, imports it and add the current user as course admin.

[OmarIthawi]

Thanks @melvinsoft. I've done a quick review and here's my comments.

[melvinsoft]

@thraxil @OmarIthawi Thank you for your reviews.

@OmarIthawi I replied inline or addressed all your comments.

@thraxil I like the backports idea, but is not available in the platform, I'd like you know your opinions if it worth to add a new requirements just for this use case.

[OmarIthawi]

A.

@melvinsoft it's up to you. The backports package sounds like a nice way to do it. Otherwise you could achieve the same with from tempfile import mkdtemp. However, you have to do the cleanup yourself shutil.rmtree.

The with tempfile.TemporaryDirectory() as dir: provides a much nicer cleanup and slight guarantees that it would do its best to cleanup in the case of an Exception for example.

B.

@OmarIthawi Sorry, I don't get this change about the GitHub URLs.

@melvinsoft I don't see why GitHub is needed, while we could use just a URL that we can put from anywhere e.g. S3 in case GitHub decided to go down for a day or two.

GitHub releases are meant to be used (in my opinion) for deployment time, but not for production and be up 100% of the time.

On the other hand, something like S3 or just a simple CloudFront cache in front of GitHub releases are by design much more resilient.

So my suggestion was to use a URL that we could configure to use GitHub or anything in the edx-configs.

[melvinsoft]

@OmarIthawi I've implemented the backports change suggested by @thraxil already, I agree, it's way more elegant.

What I don't agree is the other suggestion, it was designed to use github on purpose for several reasons, for example, if we use a bucket, we need to take care of that bucket, if some reason we re-provision or change our infrastructure, someone will need to upload the course, or several version of the course to the new bucket, also this gives more engineering independence to CS team to work, update and version the course, it also assures the course will be always in tar.gz format, has the correct name, correct folder structure, etc.
I also did a quick search about github releases availability and I didn't find anything that suggest that is less reliable that github itself.

[OmarIthawi]

@OmarIthawi I've implemented the backports change suggested by @thraxil already, I agree, it's way more elegant.

What I don't agree is the other suggestion, it was designed to use github on purpose for several reasons, for example, if we use a bucket, we need to take care of that bucket, if some reason we re-provision or change our infrastructure, someone will need to upload the course, or several version of the course to the new bucket, also this gives more engineering independence to CS team to work, update and version the course, it also assures the course will be always in tar.gz format, has the correct name, correct folder structure, etc.
I also did a quick search about github releases availability and I didn't find anything that suggest that is less reliable that github itself.

Makes sense. GitHub is reliable, but honestly don't depend on that too much, S3 in comparison is way more reliable by design.

So how about we focus on the original problem I suggested: Downloading from GitHub everytime there's a new customer is slow and error-prone.

How about the following solution:

• During deployment (aka Ansible) time download the course to /edx/var/edxapp/course-template/course.tar.gz
• Extract the course into /edx/var/edxapp/course-template/course_data
• So the task would just import the code

If that sounds too much work, it could justify doing it as a followup work after merging and testing this PR on Staging.

What do you think @melvinsoft?

[melvinsoft]

@OmarIthawi Thanks for the review and the suggestion. It makes sense, and is more robust, but I won't have time to do that work this sprint, unless I sacrifice other priorities, and this is a Q1 goal. Also, that solution is not compatible with one of the requirements, that is the ability to update the course in place, in that solution we need to re deploy edx platform in order to update the course for new sign ups.

I think the current solution is robust enough, I don't see why we should expect to many errors from Github, also isn't really slow, and it also runs in a celery tasks.

So we have two options we merged as it is, which I think is robust enough and meets the requirements, or we use a single download URL but it will cost us in engineering time on every course update.

[OmarIthawi]

Thanks @melvinsoft for sharing the full picture. I'll take another round of review to spot minor issues. Also, if it's possible to add a basic test case it would be awesome!

[thraxil]

I'd prefer the more generic URL approach rather than tieing it to github specifically, but I'm OK with pushing that off until later. It looks like it wouldn't be that big a change to make if/when we decide to.

[melvinsoft]

@thraxil Thanks for your review!

@OmarIthawi I'm planning to merge this tomorrow so we can start testing on Staging, please give another pass if you can before.

[OmarIthawi]

Thanks @melvinsoft. I don't see any blockers, but I have several tiny changes so I'm making a review PR to put my notes in.

Check it out #553

[OmarIthawi]

@melvinsoft I took the liberty of cleaning up the branch history, please let me know if you'd like me to revert the commit squash.

[melvinsoft]

@OmarIthawi no worries, thanks for squashing them and thank you both for the reviews.