Pull Requests

Pull Requests

Contents

• General Process
  • Commit Message Standards
    • Commit Types:
    • Commit Style Guide
  • Contributor: Submitting a PR
  • Reviewer: Reviewing the PR
  • Contributor:
  • Reviewer: Re-reviewing changes
  • Contributor: Merging a PR

An overview of our pull request process, including general guidelines, review processes, and merge policies.

IMPORTANT: In general tasks like rebasing or resolving merge conflicts SHOULD NOT be done using the GitHub UI. Experience shows this does not give as granular control over the outcome, and often creates weird merge commits or other undesirable artifacts in the commit history.

Key takeaways:

• The commit history on default branches must be kept tidy with commit messages that adhere to Conventional Commits.
• PR comments should adhere to Conventional Comments.
• Refer to the BeeGFS Git Workflow for guidance on which branch(es) to target based on your changes.

General Process

In general we have branch protection rules setup for branches like main, master, release-X, requiring a pull request to contribute to these branches. Our standards ruleset requires:

• At least one approval by an approved reviewer/code owner.
• Approvals are dismissed if new commits are added that change the diff from when the PR was approved.
  • This notably means you can rebase and squash changes after PRs are approved without requiring re-approval, and also be confident nothing changed unintentionally. (reference).
• All required GitHub Actions checks must pass before merging. These include automated unit and integration tests, static code analysis, security and license compliance scans, contributor verification, and other quality gates defined for the repository.
• All conversations must be resolved.

When merging PRs where multiple commits should be merged to the base branch only merge commits are allowed (not rebase merging). When using a merge commit to merge multiple commits you are responsible for first reasonably squashing down the commit history on your local system before merging. For example WIP or "fixing" or "addressing review" commits should always be squashed. When using a merge commit the commit message of the merge commit will default to Merge pull request #<NUM> from ThinkParQ/<branch> (do not change this).

Note: We do not allow rebase merging because we want to ensure there is a commit that includes the PR ID to ensure future code "archeologists" can easily find the original PR where discussion on the changes took place.

If you only need to merge a single commit or want to automatically squash multiple commits into one commit you can use the "Squash and merge" option. By default the squash commit message will include the PR title and ID as the commit message. While you can (and often should) cleanup the commit message to ensure it follows the conventional commit standards (especially the long description when squashing multiple commits together), DO NOT remove the PR ID from the commit message to ensure we can easily associate a particular commit with the PR where it was introduced.

We also automatically delete head branches after PRs are merged to keep the branch pool tidy. These can be restored anytime from the original PR if needed.

Commit Message Standards

To maintain consistency and clarity across our repositories, use the following commit message guidelines. These standards help with organization, readability, and automation when working with commits and are adapted from Conventional Commits, to make it easier to automate changelogs and improve traceability.

Each commit message should follow this structure:

<type>[optional scope]: <description>

• : A required tag that describes the nature of the change.
• [optional scope]: The specific package, module, or component the change affects (in parentheses).
• : A concise summary of the change, written in lowercase with no ending punctuation.

As a special case, when backporting a commit to a prior release (usually with a cherry-pick), prefix the commit message with [major.minor] for example [7.4] fix(ctl): some issue and mention the original commit at the bottom of the extended description in the form (cherry picked from commit abc1234). This is done automatically if you do git cherry-pick -x -e <commit-hash> which is the preferred approach and -e will also allow you to edit the commit message.

Commit Types:

Each commit should begin with a type that categorizes the change. This helps maintain clarity, particularly in projects like beegfs-go, where multiple components are consolidated.

General Types:

• feat – Introduces a new feature in code we ship to users.
• fix – Patches a bug in code we ship to users.
• docs – Updates to documentation only.
• chore – Miscellaneous maintenance (e.g., dependency updates, tooling).
• refactor – Code restructuring without behavior changes.
• test – Adds or updates tests.
• perf – Performance optimizations or general improvements not related to a bug.
• build – Build system or package-related changes.
• ci – Updates related to CI/CD.
• revert – Reverts a previous change or commit.

For changes related to a specific package or module, use an optional scope in parentheses:

Examples:

• ci(ctl): update build process
• fix(filesystem): resolve metadata sync race condition
• feat(logger): add structured logging support
• chore(deps): bump go version to 1.22
• test(mapstore): fix possible race condition testing entry creation
• perf(filesystem): optimize order of operations
• bug(logger): debug logs caused excessive allocations reducing performance

Commit Style Guide

• The first line should be all lowercase, with no ending punctuation.
• Keep the first line concise (aim for under 72 characters).
• Use the imperative mood (fix, not fixed or fixes).
• Additional details should go in the body, separated by a blank line.
  • We do not define any specific line length limits for the body of a commit. Some developers prefer to wrap at 80 or 120 characters but this is not a hard rule. In general it is recommended to use bullets with short sentences where possible to improve readability.

✅ Good Examples:

fix(ctl): resolve race condition in metadata sync
feat(ctl): add support for canceling active sync jobs
chore(deps): update dependencies to latest versions

❌ Bad Examples:

Fix: Resolved race condition in metadata sync.
chore - bumped dependencies
fix(ctl): Add better error handling.

Contributor: Submitting a PR

1. Create a new PR in GitHub:

   1. Add yourself to the "Assignees".
   2. Create the PR, optionally create it as a draft PR if it is not ready for review. Note it may sometimes make sense to add reviewers to a draft PR, but be sure to be clear about the expectations (i.e., what is "done" and safe to start reviewing versus what is still liable to change and would be a waste of time to start reviewing).

   

2. When the PR is ready for review:

   1. Ensure the previous steps are complete then unmark it as a draft if needed.
   2. Add one or more "Reviewers".

Reviewer: Reviewing the PR

TIP: You can find a list of all PRs where your review has been requested here.

1. From the "Files changed" tab, comment on specific line(s) where you have questions or want to suggest changes. The use of the conventional comments conventions are encouraged using labels to clearly establish the intention behind each comment.

   1. Prefer to use the "Start a Review" then "Add review comment" buttons instead of "Add a single comment" as this keeps notifications for the contributor tidy. Also this is nice if you decide to go back and delete a comment before submitting a review (i.e., if you later on figured out why/how something worked and the comment is not needed).

   

2. When your review is complete, use the "Finish your review" button at the top right to submit all pending comments and either "Approve" or "Request changes".

   1. You also have the option to "Comment" and just generally submit feedback without explicit approval/rejection, but there are limited situations where this makes sense.

   

Contributor:

1. Once your reviewer has submitted a review, you will see the icon by their name change to indicate if they've approved or requested changes.

   

   

2. Make changes and respond to review comments or simply "like" the comment if no response is needed to indicate you've addressed the feedback.

   1. In general it is preferred not to resolve these conversation threads so reviewers can quickly go back through their previous comments and mark them as resolved after verifying the resolution was acceptable. The exception is once the PR is approved, it is fine to go back and resolve any open threads as at this point the reviewer has indicated they are okay with the current state and remaining open threads are usually just commentary/thoughts that don't require further changes (or threads the reviewer overlooked resolving).
   2. Prefer not to rebase or force push changes as this can make it difficult for the reviewer to go back and see what was updated.

3. IMPORTANT: When you have finished addressing their feedback use the "Re-request Review" button to notify the reviewer you are ready for a second/third/etc. pass.

   1. This will place the review back in the reviewers Review Requests and also send them another notification.

   

Reviewer: Re-reviewing changes

1. Review any changes since your last review. On the "Files changed" tab their is a helpful option to filter the commits:

   

   1. If for some reason the PR was rebased and/or their was a force push, GitHub does add a "Compare" link next to any force pushes in the PR history:

   

2. Follow the same process as before to comment on the PR as needed then submit a review either approving or requesting changes.

3. Click "Resolve conversation" on threads to acknowledge you are good with how that thread was resolved or provide a follow on comment if further work is needed.

Contributor: Merging a PR

1. When the PR is ready to merge, if you want to keep more than one commit, squash down the commit history locally (including any commits generated from the review) so each commit makes sense as a standalone change. Otherwise you can just use the GitHub UI to squash everything into a single commit. We do this for a few reasons:
   1. We want to keep the commit history on main clean, especially avoiding generic "addressing review" comments. With both the merge commit and squash merging strategies there should always be a commit that includes the PR ID. That way if someone needs more detail behind a certain change, they can easily find the original PR.
   2. Ideally each commit is a self-contained change that could dropped from the main branch if for some reason we wanted to revert that functionality.
2. If there are any merge conflicts with the main branch you will also need to rebase and resolve the conflicts locally - DO NOT use the GitHub UI, it often doesn't work as expected.
3. Force push the changes to your PR. As long as your changes don't change the diff from the state when your PR was approved then the PR approval should not be dismissed.
   1. Generally the diff before/after squashing and rebasing should be the same, unless you messed something up, or you had to resolve a merge conflict (in which case you will need the reviewer to sign-off again).
4. Merge the PR in GitHub. The branch should be automatically deleted to keep a clean branch pool, but can always be restored later by clicking "Restore Branch" button at the bottom of the PR.