Skip to content

Set the MEDIA_ROOT and MEDIA_URL settings in the cms.envs.aws module - #15005

Merged
jibsheet merged 2 commits into
openedx:masterfrom
replaceafill:aws_with_filesystem_storage
Jun 9, 2017
Merged

Set the MEDIA_ROOT and MEDIA_URL settings in the cms.envs.aws module#15005
jibsheet merged 2 commits into
openedx:masterfrom
replaceafill:aws_with_filesystem_storage

Conversation

@replaceafill

@replaceafill replaceafill commented Apr 27, 2017

Copy link
Copy Markdown
Contributor

In a sandbox setup, the course export functionality throws this error if the file system is used as storage:
local storage export error
This is caused because the MEDIA_ROOT and MEDIA_URL settings are not set explicitly in the cms configuration and their default values (empty strings) are used to calculate the base location of the storage, breaking the Celery task.

This PR reuses both settings from the lms.envs.common module and allows to override them through ENV_TOKENS fixing this permission issue.

Sandbox URL:

LMS: https://pr15005.sandbox.opencraft.hosting/
Studio: https://studio-pr15005.sandbox.opencraft.hosting/

Testing instructions

  1. Set up a sandbox
  2. Set COURSE_IMPORT_EXPORT_BUCKET to null in the /edx/app/edxapp/cms.env.json file. This will make the sandbox to use the file system as storage for the course exports.
  3. Restart the supervisor service
  4. Try to export the demo course and you should see the permission error in the screenshot above
  5. Set this branch in /edx/app/edxapp/edx-platform
  6. Restart the supervisor service
  7. Exporting the demo course should work now

Reviewers

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @replaceafill! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Makes sense to me.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@gsong Can you please help us create an OSPR for this PR from OpenCraft? We're seeing an error: "This bot does not have permissions for repo 'edx/edx-platform'.\n\nPlease manually make an OSPR ticket on JIRA."

Also, CI tests have not yet run for this PR so we need to make sure that we test the jenkins build.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @replaceafill! I've created OSPR-1764 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@jibsheet

jibsheet commented Jun 1, 2017

Copy link
Copy Markdown
Contributor

If we're going to import MEDIA_ROOT from lms common, we should remove the override in cms/envs/devstack.py (especially since they should be the same setting).

Please also make a configuration PR so that step 2 is unnecessary. That should never have been needed, the default should have been local storage. Changing the ansible to pass an empty string with a comment explaining that you can set it to a bucket name would be great.

It's unfortunate that this code hardcodes S3Storage in aws.py but that's a fix for another day.

@macdiesel

Copy link
Copy Markdown
Contributor

@replaceafill Do these changes sound good?

@replaceafill

Copy link
Copy Markdown
Contributor Author

@jibsheet @macdiesel The changes sound good. I'll work on this as soon as possible.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization labels Jun 8, 2017
@replaceafill

Copy link
Copy Markdown
Contributor Author

@jibsheet @macdiesel I implemented the changes. The configuration PR is here: https://github.com/edx/configuration/pull/3906

@macdiesel

Copy link
Copy Markdown
Contributor

@jibsheet I'm going to leave this to you to merge.

@openedx-webhooks openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jun 9, 2017
@jibsheet
jibsheet merged commit eb3bbc7 into openedx:master Jun 9, 2017
@jibsheet

jibsheet commented Jun 9, 2017

Copy link
Copy Markdown
Contributor

A note - as I'm out next week, if there are issues with this on devstacks, @macdiesel will help coordinate.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, June 12, 2017.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@replaceafill

Copy link
Copy Markdown
Contributor Author

@jibsheet @macdiesel I opened https://github.com/edx/edx-platform/pull/15360 to fix the bok choy tests broken by this PR. Could you please take a look at https://github.com/edx/edx-platform/pull/15360#issuecomment-309800619 and let me know what I'm missing about running those tests? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engineering review open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants