For every task that results in new/changed code, you will create one or more pull requests ("PR", also called "merge requests" or "MR" on GitLab). 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.
When to open a pull request¶
In general, all code changes to a git repository should happen through a pull request.
What about tiny, safe changes (like fixing a typo, or changing a minor deployment setting in a configuration repository)? Yes, even then: open a pull request, don't just commit to master. In this case, you don't need to wait for a review (you can merge it right away), but it's important to still use a pull request.
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.
Why open a pull request¶
Pull requests are important for facilitating the code review process, but they do more than that:
- Pull requests send an email notification to anyone "watching" the repository, so they can stay informed about new developments.
- Pull requests will usually trigger an automated build, to ensure tests are passing, provision a test environment, or even to deploy the changes when you merge them.
- Pull requests provide a way to associate the changes with a particular JIRA ticket (this can also be done via the git commit message, but a PR title/description can be edited by anyone with admin access to the repo, and a git commit message cannot.)
- Pull requests provide a public place to give context for the changes, as opposed to JIRA tickets which are often private.
What to include in a pull request¶
When writing a pull request, you want to make it as easy as possible for everyone involved to understand the changes you're making, to review the code, and to test your changes before merging. Here are things you should include to achieve that:
Title: Give the pull request a descriptive title, and include any OpenCraft JIRA ticket IDs in square brackets as part of the title (note that you can edit the title later if needed); it's really important for OpenCraft's workflows that we can find pull requests from JIRA tickets and vice versa. We generally do not include ticket numbers from a client's private JIRA in the pull requests, although the "Yonkers" client requires that their JIRA ticket numbers be put in the branch name. We also generally don't mention the client's name unless they have asked us to credit them.
Example title: "[SE-1234] Implement UI to import a content library from a .tar.gz file"
Decisions: Remember that pull requests are not often seen after they merge - so as explained in Coding Standards you should always document major design decisions within the codebase itself (docstrings, README, ADRs) and just link to that explanation from the PR description.
edx-platform pull request template shows what is expected in a good pull request description; we recommend using this template when you open a new pull request.
If you are opening an
edx-platform pull request, Ocim will automatically provision a sandbox for your PR, using its branch. If you need to enable some specific settings (via ansible variables) for the sandbox, include a section like this at the end of your PR description:
1 2 3 4 5
Other tips on what to include:
- You may also want to review the edX PR Cover Letter Guidelines.
- Keep the recommendations from the edX 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.
- Remember who you are writing the description for: other developers and product/community managers, who may not have as much context as you do about these changes. You need to explain your changes clearly and convince the reviewers that the change provides value for the codebase. (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.)
edX Pull Request Types¶
If you're performing a pull request against an edX repository, you must keep in mind what type of pull request you're making. edX is sometimes our client and other times a community partner. The process for interacting with them differs slightly depending on the roles we are playing at that particular time.
OSPR, or 'Open Source Pull Requests', are pull requests that result from our work either with other clients or as independent contributors to the Open edX platform. OSPRs tend to be reviewed slower than blended tickets since they are unlikely to be immediately relevant to edX's use cases. They may cover fixes for bugs edX has not run into or features that one of our clients have requested and we're seeking to upstream to avoid code drift.
When creating a pull request against an edX repository, by default, an OSPR ticket will be created by edX's bot,
This ticket is created in the Open edX Jira for prioritization by the Open edX team.
BD, or 'Blended' pull requests are pull requests that result from work performed for edX as a client. These requests should be reviewed on high priority because edX is relying on the changes in these pull requests. See the main article on working with edX for more information.
How to create meaningful commits¶
For more information about how to create proper commit messages, please refer to the latest OEP-0051.
Additionally, you can read more in this article about why it matters to have meaningful commit messages.
How to open a pull request¶
- ✏️ Commit your changes on a branch, and push the new branch to the
open-craftfork of that repository.
- e.g. For changes to
edx-platform, you should generally create your branch based on the upstream
edx/edx-platform masterbranch (do not use the
open-craft/masterbranch, which is very old), and push your new branch to the
- If the repository you are modifying isn't an OpenCraft repository and hasn't been forked to the
open-craftorganization on GitHub/GitLab, you'll need to do that first. If it's a GitHub repo, after you fork it, only you will have write access by default - so go to "Settings" > "Manage Access", and add the "Core" team as "Admin" and then grant "Write" access to both "New Members & Candidates" and "Short Term".
- Exception: certain clients whose repositories are private (closed source) do not want their repositories forked. If a repository is private and no
open-craftfork exists, ask the client owner to find out where the new branch should be pushed.
- e.g. For changes to
- 💬 Open the pull request using the GitHub or GitLab UI (or app or cli tool). Use the title and template from "What to include in a pull request", above.
- 🔗 Link your PR to the OpenCraft JIRA ticket: From the ticket in JIRA, click "More" > "Link" > "Web Link" and paste the URL of the pull request. Put a description like "Upstream PR".
👩💻 Finish coding: If you've opened the pull request early (as recommended), then at this point, keep developing it until it's complete and ready for review. You can also ask your reviewer(s) to do a quick review of the work in progress if you'd like (but don't necessarily wait on that to do further work, as your reviewer may not have time to help on short notice).
- 👀 Self-review the PR: take a minute to "reset" yourself, then read through the entire PR description and code diff as though you're a reviewer seeing it for the first time. You'll probably find a few typos and things that aren't clearly explained - fix those now. If you haven't already, work through your step-by-step manual testing instructions now to confirm that the instructions are clear and the code is working.
- 🚩 Check for migrations: If the PR has introduced new migrations, you and your reviewers need to take special care and possibly split the changes up into two or three separate pull requests. See "Everything About Database Migrations" for details.
- ✅ Wait for the CI builds to complete, and address any test 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.
- 🔗 Link to the sandbox, if available. A sandbox makes it easier for reviewers to test your code. For
edx-platformpull requests, Ocim will try to create a sandbox for you automatically, but you'll still need to check that it worked and link it to the PR. For other PRs, you'll need to create a sandbox manually if possible. See "Sandbox process details" below. At this point, the sandbox may also be out of date, so make sure it's running the latest version of your code.
- 📡 Ping your OpenCraft reviewer on the JIRA ticket that this is ready for review. If there are multiple PRs, include a link to the PR in your ping. (It's not necessary to also ping them on the pull request itself.)
- 💯 Address the feedback from your reviewer, and get their approval.
- 📡 Ping your upstream reviewer (all PRs to external repositories) or core committer reviewer (only PRs to edX repositories), if you know who they are. If this is an open source contribution / OSPR, you may need to wait for edx / the upstream project to assign a reviewer. If this is an OpenCraft repository/project, skip this step and the next.
- Just as for internal reviews, the Core Committer needs the be assigned before the sprint starts to avoid injections in the Core Committer's sprint
- 💯 Address the feedback from your upstream reviewer, and get their approval.
- ✨ Squash your PR down to "one commit per major change", with a clear commit message. Put useful context from the PR as additional lines in the commit message if useful. You should also rebase on top of the latest
masterbranch while you do this, and fix any merge conflicts that may arise.
- ✅ Wait for the CI builds to complete with your squashed changes.
- 🚀 Merge the PR (if you have permission) or wait for one of your reviewers to merge it.
- 🔭 Monitor the deployment:
- For PRs to edX repositories, it will likely get deployed by CI within hours of your PR merging. Once your PR is merged, you'll get a PR comment 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.
- For other repositories, deployment processes vary. In any case, make sure it works once your PR gets to stage and prod, and try to be available in case there are any issues.
- 🗑️ 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.
- ⏭️ At this point, you may need to backport the PR to another branch/fork, depending on what client it's for. If not, you're done! Mark the JIRA ticket as Merged. Then let the client know (if applicable) and mark the JIRA ticket as "Delivered to client."
How to request Core Committer review¶
Note: This section is about edX related repositories. If you are looking for the Core Committers of OpenCraft, please look for the people next to "OpenCraft" in this table.
Getting a Core Committer review has its own workflow in Jira. The workflow is applicable for those tickets which will require upstreaming. When you need a Core Committer to review your changes, the process adjusted as the following:
- When you take a ticket, find/ask a Core Committer to do the review, and they can assign themselves using the dedicated "Core Committer" field
- When you get a Core Committer review, you may skip the usual code review ("Reviewer 1") at your decision.
- In case a Core Committer is not available or you need help communicating with edX, to ensure that the Pull Request gets the desired priority and attention from edX ping the OSPR liaison for your cell.
- Implement the required changes.
- Open a upstream Pull Request.
- Ask an optional - but recommended - code review from "Reviewer 1".
- After review feedbacks are addressed, ask a Core Committer review.
- When the changes are approved, merge the PR.
In some cases, edX will do a product review if needed. While waiting for input from edX product, still move the ticket to “External Review/Blocker”.
Sandbox process details¶
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.