fix(git-auto-export): Start export only if GIT_URL exists in the course settings #495
Conversation
marslanabdulrauf
changed the title
There was a problem hiding this comment.
I left some inline comment.
Along with these inline comments I have some other thoughts too which I came across setting this plugin up after a long time.
- The default path for
GIT_REPO_EXPORT_DIRis set to/edx/var/edxapp/export_course_repos, which does not work by default with Tutor and this was the first step installation failed for me. A more suitable path with tutor will beopenedx/course_reposbecause it works with Tutor. I tried and it works becauseopenedxis mounted into the container. - While working on point/1 I would recommend checking the settings set for
GIT_REPO_EXPORT_DIRin https://github.com/mitodl/ol-infrastructure because if that is using a default path anywhere then this change will break a few things because we are not using Tutor for our deployments yet. It would be ideal if the infrastructure uses a custom path forGIT_REPO_EXPORT_DIR. - Additionally, With this new check in place, the task will still start and stop right away if there is no Git URL. An optimization could be to place this check in
signals.pybecause why start a celery task when we already know that it will stop right away. - You will also need to bump the version in BUILD file.
Additionally, based on @pdpinch 's comment https://github.com/mitodl/hq/issues/6803#issuecomment-2741336346 where he wanted to create the repo automatically and have the Github URL set (Not in the advanced setting but our own mapping table). But, We are probably not going towards that approach at the moment we are not going towards that approach in favor of #492
| @@ -15,6 +15,13 @@ def async_export_to_git(course_key_string, user=None): | |||
| course_key = CourseKey.from_string(course_key_string) | |||
| course_module = modulestore().get_course(course_key) | |||
|
|
|||
| if course_module.giturl is None: | |||
| LOGGER.debug( | |||
| "Course %s does not have a giturl, skipping export.", | |||
There was a problem hiding this comment.
| "Course %s does not have a giturl, skipping export.", | |
| "Course %s does not have a GIT URL set in course advanced settings, skipping auto export.", |
| @@ -15,6 +15,13 @@ def async_export_to_git(course_key_string, user=None): | |||
| course_key = CourseKey.from_string(course_key_string) | |||
| course_module = modulestore().get_course(course_key) | |||
|
|
|||
| if course_module.giturl is None: | |||
| LOGGER.debug( | |||
There was a problem hiding this comment.
We should log this as info and not debug. These logs from the tasks do not show on the terminal when debug is used. Also, Debug will be false in live deployments. I do not know how you were able to see and verify these logs as you mentioned in the PR description.
fix: default git export directory path updated
|
I did a basic search in ol-infrastructur and they are using |
marslanabdulrauf
changed the title
arslanashraf7
changed the title
arslanashraf7
changed the title
What are the relevant tickets?
https://github.com/mitodl/hq/issues/6803
Description (What does it do?)
This PR:
How can this be tested?
Course {course_key} does not have a giturl, skipping export.instead of error saying'NoneType' object has no attribute 'endswith'