Pull Request¶

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 (note that you can edit the title later if needed). We generally don't mention the client's name unless they have asked us to credit them.

Example title: "feat: 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.

Template: The edx-platform pull request template shows what is expected in a good pull request description; you must always try to use this template when you open a new pull request if a specific pull request template is not available for the repository.

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:

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

Private Links: It is important that we ensure that all information which are valuable to gain context about any changes made by a commit or a PR, should be readily accessible to all members of the community through that commit/PR and not be behind any private links such as OpenCraft's internal Jira links (Ref. OEP-51#motivation). It is also equally important for OpenCraft's workflows, that we can find pull requests from JIRA tickets and vice versa. As such, we should ensure the following :

Do not include OpenCraft's internal JIRA ticket IDs or client's private JIRA IDs in PR titles.
Ensure OpenCraft's JIRA ticket IDs and the corresponding links are included in the Other Information section of the Pull Request Template with Private-ref: token. We generally do not include ticket numbers from a client's private JIRA.
Make sure any relevant information from our internal JIRA ticket, is included in the Description section or any other revelant section of the PR template.
Ensure that the subject line of commits to upstream public repos, do not contain reference to private tickets. Specifically:
- Do not include internal Jira IDs in the subject line and body of commit messages to upstream PRs.
- Do not include internal Jira IDs in PR branch names, as the subject line of merge commit messages often includes these branch names.
If required, include any private references in the Footer section (OEP-51#footer) of the commit message with Private-ref: token.

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 guide 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¶

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, edx-webook. This ticket is created in the Open edX Jira for prioritization by the Open edX team.

BD¶

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¶

Since OEP-0051 is merged, edX adopted Conventional Commits as part of the Change Transparency initiative.

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-craft fork of that repository.
- e.g. For changes to edx-platform, you should generally create your branch based on the upstream edx/edx-platform master branch (do not use the open-craft/master branch, which is very old), and push your new branch to the open-craft/edx-platform fork.
- If the repository you are modifying isn't an OpenCraft repository and hasn't been forked to the open-craft organization 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" > "Access" / "Collaborators and teams", 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-craft fork exists, ask the client owner to find out where the new branch should be pushed.
💬 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-platform pull 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 contributor 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 Contributor needs the review ticket to be assigned before the sprint starts to avoid injections in the Core Contributor'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 master branch 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 Contributor review¶

Note: This section is about edX related repositories. If you are looking for the Core Contributors of OpenCraft, please look for the people next to "OpenCraft" in this table.

Getting a Core Contributor review has its own workflow in Jira. The workflow is applicable for those tickets which will require upstreaming. When you need a Core Contributor to review your changes, the process is the following:

When you take a ticket, find/ask a Core Contributor to do the review, and create a separate Jira ticket for their review.
- Use this bookmarklet to create the ticket with the correct epic (same as for the main ticket), account (same account as main ticket), label (CoreContributor), and link. Assign the new ticket to the Core Contributor who can review it, or have them use the bookmarklet to create their own ticket.
- Note for Core Contributors: when installing the bookmarklet, make sure to edit line 4 to set your cell.
- Whenever possible, the Core Contributor review ticket should be in the same sprint as the main ticket, and you should finish the implementation early enough for the Core Contributor to review and merge it within the sprint.
- In case a Core Contributor 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 Community Liaison.
Implement the required changes.
Open a upstream Pull Request.
Ask for an optional - but recommended - code review from "Reviewer 1". (When you will be getting a Core Contributor review, you may skip the usual code review ["Reviewer 1"] at your discretion.)
After review feedbacks are addressed, ask a Core Contributor 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.

How to handle Code Drift¶

If you open a PR against one of the branches/forks under open-craft, for example: opencraft-release/palm.1, to implement a specific change for a client, that results in a code drift. We must stay on top of code drifts and upstream all the changes we make by default, as they can become cumbersome over time, especially when a new release is cut. In cases where the upstream PR takes too long to be reviewed, is rejected, or cannot be upstreamed for some reason, all the changes need to be cherry-picked into the new release to remain supported.

To help with minimizing code drift and keeping it under control, we have 2 processes in place:

Code Drift Github Project: This project board¹ gives us a holistic view for the state of the code drift for repositories/forks under the open-craft organization. Whenever a new PR is created against a branch in the open-craft/edx-platform, it is automatically added to the board with Status set to Missing upstream PR (the default column). The Status column has the following states:
- Won't upstream: This is for PRs that we cannot upstream. If you use this status, provide as much context as possible in the Notes field.
- In progress: The PR is not ready for review yet.
- Missing upstream PR: We still need to create an upstream PR.
- Upstream review: The upstream PR is waiting for review.
- Merged into master: The upstream PR was merged to master, but not cherry-picked to the next release.
- Present in [upcoming release]: The change is already present in the next named release.
The PR author is responsible for filling in the remaining fields and following this process. A central place for identifying the existing code drift greatly simplifies creating named release branches. It also helps us track ongoing contributions.

The example below assumes that the currently used release is Nutmeg, and the upgrade to the existing Palm release is ongoing. The upcoming releases are Quince and Redwood.
1. The assignee opens a PR against our Nutmeg branch. The GitHub project automatically adds a new entry with the Missing upstream PR status.
2. The assignee adds the client name (and, optionally, the notes) to this new entry.
3. If the PR is not ready for review, the assignee changes its status to "In Progress".
4. The assignee opens a PR against the upstream repository, manually adds a new entry to the board, and updates its fields. Then, the assignee removes the internal PR from the board.
5. Once the upstream PR is merged, the assignee sets the PR status to:
  1. Merged into master - if the upstream Quince branch already exists.
  2. Present in Quince - otherwise. The assignee should also cherry-pick the change to our Palm branch, if appropriate.
6. If the change cannot be upstreamed, explain why in the Notes and set the PR's status to Won't Upstream. Doing this is discouraged - we must have a very good reason not to upstream a PR.
7. Once we no longer have Nutmeg instances, the owner of the "Palm Upgrade" epic will rename the Cherry-picked to Palm? column to Cherry-picked to Quince?. They will set the value of its fields to "No" for all items other than Present in Quince.
8. Once we create the "Quince Upgrade" epic, its owner will:
  1. Remove all items with the Present in Quince status.
  2. Rename the Present in Quince status to Present in Redwood.
  3. Update all items with the Merged into master status to Present in Redwood.
Github Action to synchronize custom branches with upstream: This Github Action is currently scheduled to run every Monday 00:00 UTC. It checks if there are any new changes in the upstream named release branch and creates a PR against our fork accordingly. It is currently set up for the Palm release but can easily be extended to support more releases/forks. This Github Action aims to automate the manual steps involved with keeping a fork branch up-to-date with upstream. PRs are only created when there are new changes upstream. A reviewer has to review the changes, resolve any conflicts, then merge the PR, as explained in its description.

open-craft/edx-platform#575 is an example PR created by this GitHub Action.

WARNING: When merging PRs created by this Github Action, always use the "Create a merge commit" option. Since this action depends on the commit hashes to determine if there are any new changes upstream that need to be synced, if a rebase is performed, you will change the commit hash and this action will not be able to detect that the changes were merged.

Private for OpenCraft Employees ↩

Last update: 2023-12-14