Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load OVA JS with RequireJS [TNL-693] #6158

Closed
wants to merge 29 commits into from

Conversation

jmclaus
Copy link

@jmclaus jmclaus commented Dec 5, 2014

Note: this PR supersedes

https://github.com/edx/edx-platform/pull/5829

that will soon be closed.

Same for the branch, it supersedes:

jmclaus/load_ova_js_from_templates_with_requirejs

@jmclaus
Copy link
Author

jmclaus commented Dec 5, 2014

@polesye @tymofij @olmar Please review. Especially require-config-lms.js that is tricky.

@jmclaus jmclaus changed the title Load OVA JS with RequireJS Load OVA JS with RequireJS [TNL-693] Dec 5, 2014
@andy-armstrong
Copy link
Contributor

@jmclaus What was the thinking behing only merging this to your branch? I'd love to see the RequireJS changes get into master so we can start building on them. In particular, I'd really like to see @wedaly's PR get in once the caching issues are resolved, and I think that is dependent upon this (https://github.com/edx/edx-platform/pull/6036).

@cahrens
Copy link

cahrens commented Dec 5, 2014

Agreed, I'd rather this go to master so we can start building on it.

@jmclaus
Copy link
Author

jmclaus commented Dec 6, 2014

@andy-armstrong @cahrens

What was the thinking behing only merging this to your branch?

That using requireJS for OVA is only needed for edxnotes. And also that to be on the safe side, https://github.com/edx/edx-platform/pull/5829 should be also tested once again on a CDN-enabled sandbox, now that we have their backend storage connected. We only did a test without that access and there were some errors, that seemed related only to the fact we weren't using their backend storage. But who knows. We were going to move on in the feature branch in the meantime. Perhaps I am being too cautious.
True, we have to start using requireJS in the LMS (one of the items in Q3) and might as well start with OVA. I have no objections in merging https://github.com/edx/edx-platform/pull/5829 after some extra testing.

@polesye @tymofij @olmar, what do you think?

In the meantime, let me open a devops ticket asking to create a CDN-enabled sandbox:

https://openedx.atlassian.net/browse/DEVOPS-704

@polesye
Copy link
Contributor

polesye commented Dec 8, 2014

@jmclaus We already have a thumbs up from @lduarte1991. So, I agree with merging to master.

@jmclaus
Copy link
Author

jmclaus commented Dec 9, 2014

Closing this PR. We decided to merge https://github.com/edx/edx-platform/pull/5829 instead.

@jmclaus jmclaus closed this Dec 9, 2014
@benpatterson benpatterson deleted the jmclaus/load_ova_js_with_requirejs branch August 2, 2016 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants