Pull request process

For every task that results in new/changed code, you will create one or more pull requests. Each pull request will always go through an internal review by another OpenCraft developer, and will usually go through an upstream review process as well.

How your pull request should be structured and how your code will get reviewed, merged, and verified will vary depending on which client the code was developed for, and which organization owns the upstream repository.

Notation used in this document

When talking about branches generally, I will use GitHub's notation used on pull requests. This notation includes the name of the organization and the branch, but skips the name of the repo, as it is self explanatory.

This way organization/repo/branch is shortened to organization:branch. This allows us to talk about branching in general, or spare repetition when talking about branches of the same repo.

Some examples:

  • edx:master means the master branch in the edx organization. In case of edx-platform it is equivalent to edx/edx-platform/master.

  • open-craft:your-branch similarly translates to open-craft/actual-repo/your-branch.

Expectations

  • Do your best to ensure you've written clean, maintainable code that has good test coverage.
  • Follow the coding guidelines outlined by edX (Python, JavaScript) and use pep8, pylint and jshint to check your code. We use these guidelines for most all of our work; not just for edX projects.
  • Good test coverage doesn't necessarily mean perfect branch coverage, but instead means code is covered proportionally to how much it will be used.
  • After you have committed your code in Git (or after pushing to GitHub), read through the diff.
  • In the PR title, include the OC-XXXX and/or the upstream ticket ID to make locating the relevant ticket easier. The edX PR template below also has a place for the OC-XXXX ticket under "JIRA tickets", but since our tickets are not public, we can't link to them there.
  • Write a clear, concise pull request description that explains what your code change is doing
  • If the UI was changed, include screenshots of the changes.
  • Also think about who you are writing the description for upstream. You need to convince the reviewers that the change is good for the platform as a whole, besides our specific use case. For example, if your PR to add support for SWIFT generalizes the storage backend, the latter benefits everyone, not just SWIFT users -- focusing your PR title and description on it will be more appealing to reviewers who don't use SWIFT.
  • Remember that pull requests are not just used to facilitate code review - they will also be a reference for developers in the future who use tools like git blame to find the PR that introduced certain code.
  • If there are related issues in a public JIRA tracker, then link to those issues. Don't link to private JIRA tickets. Enterprise Team (ENT) is fine, though.
  • Monitor any CI builds, and address any failures. Your reviewers should not have to point out failed builds.
  • Check if the build passed, but also check test coverage reports generated during the build, if available.
  • Do not squash your commits during the review processes. This makes it very difficult for reviewers to "pick up where they left off".
  • Whenever you rebase and/or squash your commits, add a comment on you PR saying "Squashed from (old branch tip hash)." or "Rebased (old branch tip hash) onto (master).". Since GitHub preserves all git objects, this will allow people to go back and view the unsquashed / pre-rebase version if necessary.
  • Always squash your PR down to "one commit per major change" before merging to edx-platform
  • List any concerns, open questions, TODOs, tech debt, and known issues in the PR description
  • Include references to previous discussions - make sure to discuss ahead of time with reviewers anything which might cause issues during the upstream review. This is encouraged by upstream reviewers; be proactive and feel free to ping them on JIRA, Slack, or a WIP PR to ask questions in advance.
  • Include instructions for testing
  • Link to the sandbox, if any. Specify what commit hash was used for the sandbox.
  • Delete the branch after the PR has merged (use the delete button GitHub adds to the end of the PR). Otherwise, repos get too cluttered with old branches.

Typical PR Process

Generally you create a fork under the open-craft organization (if such a fork does not already exist) and create a new branch for your changes (open-craft:your-branch). It is often helpful to prefix the branch name with your name/username, e.g. (open-craft:braden/new-feature). Base the new branch on the latest upstream branch you are going to be targeting with your PR. (This avoids having to bring your branch up-to-date with the target branch later on; e.g. if you had mistakenly based your changes on open-craft:master but your PR needs to target edx-solutions:master, you could have to rebase your changes before the PR can be reviewed and/or merged.)

Open PRs early! Your code does not have to be complete before you open a PR. It's usually a good idea to open the PR early and push your commits regularly - not only does this keep a backup of your work, but it helps your reviewer give early feedback and get a sense of how complex the code review will be.

If your pull request is against a fork of some project, then once it has merged, you should open a second pull request to contribute the change to the upstream/original project. There needs to be a good and strong reason to not contribute a change upstream. If a change isn't mergeable upstream, it usually means it doesn't match the standards of that project, and needs to be improved to match them. Code drift needs to be maintained forever, requiring wasted work that doesn't benefit anyone. Check with your reviewer and Braden before considering not contributing a change upstream.

For specific GitHub organizations/repositories, we need to follow specific processes:

GitHub Organization: edX

The official organization for edX on GitHub is edx. The following instructions apply to any work done on repositories in the edx organization, such as:

Fork/branch: Work on open-craft:your-branch, as usual.

Merge target: Once done, create a pull request towards edx:master from open-craft:your-branch.

Guidelines & Requirements:

  • Keep the recommendations from their contribution document in mind. For the individual contributor agreement, you are submitting code on behalf of OpenCraft, so that is inapplicable; you don't have to sign it.
  • Every PR against an edx repository requires at least two reviewers to approve it, and often more. This should usually be one reviewer from OpenCraft and one reviewer from edX.
  • New/updated code needs to have good test coverage (including JS tests and bok-choy Selenium tests when applicable)
  • The Jenkins build on the PR needs to show that all tests are passing (a "green build")
  • Squash your commits (using git rebase -i) before merging (but after the code review is complete). For most PRs, squash them down to a single commit. For large PRs, use your judgment and squash them down to a reasonable number of commits, with one commit per major change.

What to put in the PR description

Use a title like "OC-XXXX Display content from a library in a course", and a description based on this template:

Description goes here. e.g. This PR contains the LibraryContent XBlock, which allows to display library content in a course.

**JIRA tickets**: If there is a related publicly viewable JIRA ticket, mention that here. Also mention the OSPR ticket
number here once it gets created (after you open the PR), if applicable. Otherwise omit this.

**Discussions**: Link to any public dicussions about this PR or the design/architecture. Otherwise omit this.

**Dependencies**: None

**Screenshots**: Always include screenshots if there is any change to the UI.

**Sandbox URL**: TBD - sandbox is being provisioned.

**Merge deadline**: "None" if there's no rush, "ASAP" if it's critical, or provide a specific date if there is one.

**Testing instructions**:

1. Step by step manual setup/testing/verification instructions go here.
2. Step 2

**Author notes and concerns**:

1. We are aware that there is a glitch affecting the UI for this feature in the following way: ...
   Currently looking for ways to fix it.
2. We tried to optimize for accessibility, but there are still some open questions: ...

**Reviewers**
- [ ] (OpenCraft internal reviewer's GitHub username goes here)
- [ ] edX reviewer[s] TBD

**Settings**
```yaml
EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true
```

Settings: Defines ansible variables used for the automatically generated sandbox - you can usually leave that section out entirely.

Sandbox:

  • If you open a PR against edx-platform, Ocim will automatically provision a sandbox for that PR. Once the sandbox has been provisioned (it takes about an hour), update the PR description to include links to the sandbox.
  • If you open a PR against a different repository:
    • Manually create a sandbox in Ocim. Include the ID of the JIRA ticket ("OC-1234", "BIZ-5678", etc.) in the name of the sandbox.
    • Link the instance to a PR from Ocim's Django admin (you may want to create a new watched pull request without linking the sandbox to it first, and use the Django shell to locate and link the sandbox to the watched pull request in a second step - this might be easier than locating the instance in the Django admin UI for creating/editing watched pull requests). Ocim will regularly check the PR's status and automatically archive the sandbox as soon as the PR is closed. If you didn't link the instance to a PR, you need to make sure to archive all sandboxes before moving a JIRA ticket to "Merged" (e.g. put it in a checklist item on the ticket to help you remember).
    • Update the PR description to include links to the sandbox.

Title & JIRA Tickets: When considering inserting the associated JIRA ticket into the title or description, always do so for publicly viewable tickets. As for OC/MCKIN tickets, always mention the ticket number, but try not to link to the actual ticket. Whenever we open a PR, the edX OSPR (Open Source Pull Request) bot will also create a new OSPR ticket in edX's JIRA. Since OSPR tickets are created only after the PR is opened, you can include it in the description after it's generated.

You may also want to review the edX PR Cover Letter Guidelines.

OSPR Review Process

For every pull request we open against an edX repo, there will be two distinct rounds of code review:

Internal Review

We almost always go through an internal review step separate from upstream review for upstream PRs. Whoever's assigned your reviewer for the internal task (i.e. OC-XXXX) will review your code, give it a thumbs up if all's good, and you and/or the reviewer should notify upstream that this is ready for their review.

Upstream Review

The upstream review process step is similar to internal review, but expectations may be different, and this time you may be subjected to cross-team review, possibly UX testing from experts, etc., depending on the scope of your task.

Extra Info

  • If the PR has introduced new migrations, let edX DevOps know about it as early as possible in the process.
  • It's best to let someone from edX do the actual merge, so they are responsible for confirming the PR has had sufficient review. (But there's no policy preventing people on our team who have merge permission from merging the PR if all required reviewers have approved it, so that's OK when necessary.)
  • Once your PR is merged, you'll get a notice on GitHub when it has been deployed to the edX stage server. When you see that, you should verify it on the stage server to make sure it's working as expected.

GitHub Organization: edX Solutions

For work we do for Yonkers that will merge into the repositories on the Github organization 'edx-solutions`, such as:

Fork/branch: Work onopen-craft:your-branch, as usual.

Merge target: Once done, create a pull request towards edx-solutions:master from open-craft:your-branch.

See the documentation about client aliases.

Guidelines & Requirements

  • Always include the relevant MCKIN ticket ID in the PR title (e.g. "MCKIN-9876 Fix bug with progress calculation")
  • Code to the same standard as PRs for the edx organization
  • If the repository has TravisCI or CircleCI running tests, the build must be green before merging.
  • Every PR requires two reviewers to approve it before being merged. Both reviewers can be from OpenCraft.
    • Exception: If the PR involves ansible or affects deployments significantly, you should get edX DevOps to review it as well.
    • Exception: If the PR is only updating the git hash of a requirement (e.g. in edx-platform/requirements/github.txt), and all changes between the old hash version and the new hash version have been reviewed in other PRs, then you can merge the PR with no review. (Still make sure the CircleCI build is green though.)
    • Exception: If the PR could affect performance of the site, performance testing may be required before merging.

Post-merge follow-up

If there is an upstream repo from which the one in edx-solutions was forked, be sure to create a second pull request against that repo too (so that the changes are propagated upstream.)

For changes to the solutions edx-platform fork: When its master branch is updated, the Yonkers QA server creates a new build, which is available at https://qa.mckinsey.edx.org/ . The studio for this build is available at https://qa-studio.mckinsey.edx.org/ . See the file with login information in this repository to access it. The build takes 30-60 minutes. After you merge a PR to edx-solutions/edx-platform:master, wait one hour and then verify that the changes are working as expected on the QA server. This way, you can get any fixes in before waiting on the Yonkers QA team, should the regression testing courses reveal anything unexpected.

GitHub Organization: OpenCraft

This organization holds our private repos and forks of almost everything we work on.

When working on our private repos, we don't use a fork; we all work from branches in the same main repo. Create a new branch as usual, push it to open-craft:your-branch, and then create a pull request against open-craft:master.

Be careful to pull from the more up-to-date original repos instead of the open-craft forks when available.

Review Requirements: Only one review is required. The reviewer should make sure any changes include updated documentation where applicable (which may be simply a README file).

GitHub Organization: Yonkers

This organization hosts tools specific to Yonkers, like apros. See the documentation about client aliases.

Fork/branch: Yonkers expect you to create a branch in their own repo in the format of username-your-branch and push your work there, they do not want anyone to create any fork of the repos under their organization.

Merge target: Unlike most other repos we work with, pull requests are done against development (not master, which is kept in sync with what's deployed on their prod servers). Occasionally, you might be asked to create a PR against another branch, such as release.

Review Requirements: In general, only one review is required, and it can be from OpenCraft. Sometimes we need additional reviews from ArbiSoft or edX though, depending on what impact the PR will have once merged.