Skip to content
This repository was archived by the owner on Aug 22, 2022. It is now read-only.

Set EDXAPP_IMPORT_EXPORT_BUCKET to null - #206

Merged
pomegranited merged 3 commits into
open-craft:masterfrom
replaceafill:OC-2479
Jul 10, 2017
Merged

Set EDXAPP_IMPORT_EXPORT_BUCKET to null#206
pomegranited merged 3 commits into
open-craft:masterfrom
replaceafill:OC-2479

Conversation

@replaceafill

Copy link
Copy Markdown

When using ephemeral databases this avoids S3 to be used as default storage for the CMS course import/export functionality.

@@ -0,0 +1 @@
EDXAPP_IMPORT_EXPORT_BUCKET: null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@replaceafill Should this be !!null ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@bradenmacdonald Ah thanks! I didn't know about the !! syntax. Just updated the branch.

@bradenmacdonald

Copy link
Copy Markdown
Member

Looks good, @replaceafill !

Conditional 👍 if you rebase and get the CI build to pass

  • I tested this: spawned an ephemeral instance on my devstack and confirmed that the combined vars had EDXAPP_IMPORT_EXPORT_BUCKET: null (which I have also tested using https://studio-pr15005.sandbox.opencraft.hosting/ )
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a

@bradenmacdonald

Copy link
Copy Markdown
Member

@replaceafill I triggered a build for you. I'm not sure if our builds are working right now, but we'll see what happens.

@bradenmacdonald

Copy link
Copy Markdown
Member

This is blocked until https://github.com/edx/edx-platform/pull/15101 is merged. Once that's merged, I'll cherry pick it into opencraft-release/ficus.3 (or just use opencraft-release/ficus.4 if they release it), then update the integration tests to use that, which should fix the build, then we can merge this.

# Workaround for broken CMS course export/import
# caused by https://github.com/edx/edx-platform/pull/14552
template = loader.get_template('instance/ansible/ephemeral.yml')
return template.render()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@replaceafill Can we make sure to revert once the upstream PR is merged? To ensure we don't forget, I would only close the current ticket once both the upstream PR is merged and this reverted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@antoviaque This is actually more a fix than a workaround, and it has to be here as well as the upstream PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, right - forget my comment then. Sorry for the noise.

@pomegranited

Copy link
Copy Markdown
Contributor

From @bradenmacdonald's comment above:

Could you rebase your pull request on master, and see if that fixes the test failures?

@replaceafill

replaceafill commented Jul 5, 2017

Copy link
Copy Markdown
Author

@pomegranited I rebased this from 2dbbfe9. It's been a while now and I don't see any ci/circleci status yet. Can we re-trigger the ci/circleci check?

@pomegranited

Copy link
Copy Markdown
Contributor

@replaceafill I've triggered a build: https://circleci.com/gh/open-craft/opencraft/2189

Not sure why it's not showing up here though.. I might have done that wrong. But let's see how it goes.

@@ -61,7 +61,10 @@ def get_storage_settings(self):
Only needed when not using ephemeral databases

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.

@replaceafill nit: Could you remove this comment? It doesn't apply anymore, since you've added ephemeral.yml.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@pomegranited Removed. I also changed the EDXAPP_IMPORT_EXPORT_BUCKET setting to an empty string to be consistent with the change we made to the configuration repo.

@pomegranited
pomegranited merged commit 6e57f5d into open-craft:master Jul 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants