Skip to content

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

Merged
cahrens merged 1 commit into
openedx:masterfrom
open-craft:replaceafill/aws_with_filesystem_storage_tested
Jul 6, 2017
Merged

Set the MEDIA_ROOT and MEDIA_URL settings in the cms.envs.aws module#15360
cahrens merged 1 commit into
openedx:masterfrom
open-craft:replaceafill/aws_with_filesystem_storage_tested

Conversation

@replaceafill

@replaceafill replaceafill commented Jun 20, 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:

df296f80-2b6b-11e7-9dfb-b4e744039c74

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://pr14831.sandbox.opencraft.hosting
Studio: https://studio-pr14831.sandbox.opencraft.hosting

Testing instructions

  1. Set up a sandbox
  2. Try to export the demo course and you should see the permission error in the screenshot above
  3. Set this branch in /edx/app/edxapp/edx-platform
  4. Restart the supervisor service
  5. Exporting the demo course should work now

Note: This change was first created under https://github.com/edx/edx-platform/pull/15005, but had to be reverted because the bokchoy tests failed.

@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.

@replaceafill

Copy link
Copy Markdown
Contributor Author

@estute I created this PR to fix the failing bokchoy tests of https://github.com/edx/edx-platform/pull/15296. I can see the same failures here too.

However, I haven't been able to reproduce them in my devstack. I first tried running the tests just for the studio/test_import_export.py module like this:

edxapp@vagrant:~/edx-platform$ paver test_bokchoy -t studio/test_import_export.py

which produced no failures: https://pastebin.com/9Sa8qs15

And then, after inspecting the Jenkins logs, using shards like this:

edxapp@vagrant:~/edx-platform$ paver test_bokchoy --attr="shard=7" --with-flaky --with-xunit

which didn't produced failures either: https://pastebin.com/D7Um2y62

How should I run the tests in my devstack in order to get the same failures Jenkins reports? Thanks!

@replaceafill
replaceafill force-pushed the replaceafill/aws_with_filesystem_storage_tested branch from 815ceb0 to eb0ae86 Compare June 22, 2017 22:14
@replaceafill

Copy link
Copy Markdown
Contributor Author

jenkins run bokchoy

@replaceafill
replaceafill force-pushed the replaceafill/aws_with_filesystem_storage_tested branch from eb0ae86 to c567add Compare June 26, 2017 01:47
@replaceafill replaceafill changed the title [WIP] Set the MEDIA_ROOT and MEDIA_URL settings in the cms.envs.aws module Set the MEDIA_ROOT and MEDIA_URL settings in the cms.envs.aws module Jun 26, 2017
@replaceafill

Copy link
Copy Markdown
Contributor Author

@jibsheet @estute Could you please take a look? This PR fixes the bokchoy failures of https://github.com/edx/edx-platform/pull/15005

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @replaceafill! I've created OSPR-1807 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.

@antoviaque

antoviaque commented Jul 4, 2017

Copy link
Copy Markdown
Contributor

@jibsheet @estute Will you be able to have a look at this soon? We would like to have this merged for Ginkgo.

@pomegranited pomegranited 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.

Works great! Thanks @replaceafill .

👍

  • I tested this using the described instructions. I also tested importing the exported course, just to be sure.
  • I read through the code
  • I checked for accessibility issues No change to the user-facing GUI.
  • Includes documentation - contains some docstrings, but it's a bugfix, so doesn't require user-facing docs.

@cahrens

cahrens commented Jul 5, 2017

Copy link
Copy Markdown

@jibsheet it looks like you reviewed the earlier version of this. Are you able to OK this PR, or does it require an explicit Educator review?

@jibsheet

jibsheet commented Jul 6, 2017

Copy link
Copy Markdown
Contributor

@cahrens I'd love someone else to look at the bok_choy.json change that fixed tests. I'm fine with the rest of the prod-facing changes, but don't have as much bok choy familiarity to be sure about the test fix.

Comment thread cms/envs/bok_choy.py
(TEST_ROOT / "staticfiles" / "cms").abspath(),
]

DEFAULT_FILE_STORAGE = 'django.core.files.storage.FileSystemStorage'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It appears that these are the same settings that are already in lms/envs/bok_choy.py. Therefore I think this change makes sense.

https://github.com/edx/edx-platform/blob/master/lms/envs/bok_choy.py#L67

FYI @jzoldak, in case you have any concerns.

@cahrens

cahrens commented Jul 6, 2017

Copy link
Copy Markdown

@jibsheet It appears that the changes to bok_choy.py just duplicate what already is in the lms version of bok_choy.py. So I think this is fine-- I have tagged @jzoldak just to be sure.

If there are no concerns, I will merge this later today.

@pomegranited

Copy link
Copy Markdown
Contributor

Thanks @cahrens and @jibsheet !

@cahrens
cahrens merged commit 8fb8647 into openedx:master Jul 6, 2017
@replaceafill
replaceafill deleted the replaceafill/aws_with_filesystem_storage_tested branch July 6, 2017 16:41
@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 Friday, July 07, 2017.

@nedbat

nedbat commented Jul 7, 2017

Copy link
Copy Markdown
Contributor

This merge was after the release that became Gingko. This will need to be cherry-picked if it's to be part of Ginkgo.

@nedbat

nedbat commented Jul 7, 2017

Copy link
Copy Markdown
Contributor

Since it was merged, I went ahead and cherry-picked it.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

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.

9 participants