MIT CCX add ccx-keys as a dependency of edx

[cewing]

This PR adds a dependency to the edx-platform repository on a newly created package ccx-keys, an implementation of opaque-keys for the Custom Course for EdX extension first accepted into edx in https://github.com/edx/edx-platform/pull/6636

History

The request to implement custom opaque keys for ccx was first brought up in an architecture review of the PR referenced above. It was felt that using such keys would allow ccx to better integrate with existing logging and analytics systems. The outcome of this conversation was captured in jazkarta#43

Work on implementing the package itself has been undertaken with attention from @cpennington. It was approved and has been managed by @pdpinch of MIT. @nedbat and @ormsbee have also been pulled into discussions around bugs and implementation details.

As this pull request adds a new dependency to the edx-platform repository, we would also like to beg the attention of @mollydb and @e0d.

Impacts

As it stands, this change will impact no-one. The keys themselves are not yet used in the CCX code in edx-platform. This PR is intended as a prelude to a coming PR where the change over to using ccx-keys will be implemented.

Once implemented the changes will be limited to the LMS. The only user-visible changes will be the presence of ccx identifiers in split-mongo compatible URLs. As ccx is only currently released to edge, the effects will only be felt there.

Testing

The only manual test possible for this change is to install the dependencies of edx-platform and verify that the package is downloaded. There should be no change whatsoever in functionality at this stage. All functions and features of EdX, both in studio and lms should continue to work exactly as before.

Urgency

As a second pull request with the changes to CCX implementing the use of ccx-keys is anticipated during the weekend (5/31/2015), it would be good to have this PR land as soon as possible to simplify the review of that work.

[openedx-webhooks]

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

[sarina]

As this pull request adds a new dependency to the edx-platform repository, we would also like to beg the attention of @mollydb and @e0d.

Since you're adding a new dependency of a library that you've created yourself, and licensed with the GPL v3 license, I think we're definitely a-OK from the licensing perspective :)

[sarina]

@cewing - can you create and post a dummy pull request on that repo, so that reviewers can review the new package?

[sarina]

@cewing actually perhaps you've already done that with jazkarta/obsolete-ccx-keys#1 - yes?

[pdpinch]

This has already been done, and Cale has done some review:

jazkarta/obsolete-ccx-keys#1

On May 29, 2015, at 11:11 AM, Sarina Canelake [email protected] wrote:

@cewing - can you create and post a dummy pull request on that repo, so that reviewers can review the new package?

—
Reply to this email directly or view it on GitHub.

[sarina]

@pdpinch yeah I saw that as soon as I looked /facepalm

I'm putting this into the Platform team's work queue for them to pick up.

[cpennington]

I still see some outstanding comments in the ccx-keys repo, but otherwise, this looks fine.

[pdpinch]

@cewing informed that the pull mentioned above is out of date. He'll make another PR for review purposes.

On May 29, 2015, at 12:32 PM, Calen Pennington [email protected] wrote:

I still see some outstanding comments in the ccx-keys repo, but otherwise, this looks fine.

—
Reply to this email directly or view it on GitHub.

[cewing]

Oddly enough @cpennington, the one remaining comment was against an outdated diff, but was not showing itself to be so. I think github lost track somehow.

I've create a new PR here:

https://github.com/jazkarta/ccx-keys/pull/2

@sarina hopefully this will address any remaining questions. Let me know what more needs to happen. I stand ready!

[sarina]

@cewing the review is in the platform's team backlog to balance with their other reviews and work commitments. The team will get to it as soon as they can.

[cewing]

Thanks, @sarina

[cpennington]

Needs an update since the latest changes on jazkarta/obsolete-ccx-keys#2, but e6b0370 looks good.

[cewing]

Thanks for the reminder. Will update this afternoon my time.

[cewing]

@sarina, @cpennington, sorry for the delay. Updated and ready for review.

[cewing]

I don't think either of the test failures showing there can be related to adding a new dependency. I'll rebase to the latest master and see if that clears up the problem.

[sarina]

@cewing yeah that's weird. Can you please squash your commits down to one and rebase atop master?

[cewing]

@sarina done. Waiting for build results

[cewing]

@sarina: woot! build is green!

[sarina]

@cpennington can you merge this, if it's good by you, or suggest what needs to happen? Thanks