Skip to content

Add guidelines on PR + commit text #311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Jun 1, 2020
Merged

Add guidelines on PR + commit text #311

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions guidelines/commit_and_pr_text.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# Motivation
Our changelogs (reviewed by users/customers) are autogenerated from PR titles and git commit text. The better this information is, the more we help our customers. This information also helps:

* reviewers understand changes
* current+future team members understand the history of changes
* patch managers understand risk/importance
* when hunting for regressions, helps us better understand what is/isn’t likely to be a cause.

# PR Metadata

__PR title:__ Bug NNNN: A short sentence with capitalization and no period or the Jira tag for the issue “Feature TEAM-XXX:”
Copy link
Contributor

@lilic lilic May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem for monitoring team is we have a lot of dependancies that are bumped with one single PR, when updating jsonnet deps, so how would we describe this in short? We now say something vague like "Bump dependencies". What would you suggest for us to write here instead?

Copy link
Contributor Author

@bparees bparees May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a totally reasonable summary sentence for a dep bump.

however if there's anything special worthy of note about the bump, it can be put in the longer description (e.g. did you have a pin a dep version or add a new one? did other code have to refactored in reaction to the bump, in which case those changes should probably be a separate commit in the same PR?)

Copy link

@soltysh soltysh May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like openshift/cluster-kube-controller-manager-operator#406 (comment), maybe not necessarily as detailed.

Copy link
Contributor

@lilic lilic May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that makes sense. Any objections @openshift/openshift-team-monitoring maybe we can just find a way to somehow copy over the commits from the upstream deps, e.g. kube-mixin commits?


* Try to describe the problem the user sees, rather than the code change you are doing
* I.e. “Console does not work when user clicks X” instead of “Fix drawer notification integration with underlying controller”
* Prefix bug fixes with the bug number, “Bug XXXXX:”
* (capitalize bug, space, number, then colon)
* Do not let your PR title get truncated/wrapped into the description
* Additional prefixes like what release the PR applies to are allowed, but they will be stripped from the changelog (the changelog for 4.4 doesn’t need to say “release-4.4”
* Changelog strips anything between square brackets that has no spaces - i.e. [release-4.4] is stripped but [release 4.4] would not be
Copy link
Member

@sdodson sdodson Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to confirm, which process actually does this as I only knew this to be the practice of patch managers but not an automated process?



__PR description:__ Additional context reviewers need to understand your PR
Copy link
Contributor

@russellb russellb May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add:

  • All of this information must also reside in commit messages, and not only in the PR summary and description

Copy link
Contributor Author

@bparees bparees May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's covered by the git commit text section below. I don't want to say that the two should be identical because of things like:

Avoid referencing github issues/PRs if you are repeatedly amending the commit as it generates a lot of noise in the referenced issue/PR.

Copy link
Contributor

@russellb russellb May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I don't think they should be identical either. What I don't like seeing is good detail in a PR summary but single-line unhelpful commit messages.


* Why was this change needed?
* What changes are being made?
* Reference to any prior work(other PRs) or follow up work that will be needed.
Copy link
Contributor

@jmrodri jmrodri May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I try to make my commits as meaningful as possible. Often like an email where the first line is the Subject. The description is more details about what is going on in the PR, motivations links to other related works, etc.

Copy link
Contributor Author

@bparees bparees May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then you don't need this doc because you're doing it right already :)


[Canonical PR text example](https://github.com/openshift/openshift-apiserver/pull/69)

# Git Commit Best practices

## Number of Commits

Give careful consideration to the commits in your PR. Multiple commits may make sense, but should each have a clear, logical, independent function. Careful commit structure within a PR makes it easier to review the PR, easier to revert the PR if necessary, easier for someone doing “git blame” to follow the set of involved changes, and encourage smaller “chunks of work” (if it’s multiple commits, think about whether it should also be multiple PRs)

Valid reasons to have separate commits:

* Separating implementation work from generated code or vendor updates
* Separating api work from implementation
* Moving files around in a refactor (do the file move in one commit, edit the files in the second commit, so that the edits stand out in the commit diff)
* Temporarily adding additional commits in response to reviewer feedback so reviewers can easily see how their feedback was addressed. These commits should be squashed prior to merging.

Invalid reasons to have separate commits
* Separating tests+implementation
* Committing+pushing your work each day/hour (use amend instead, or at least ensure you squash before merging).
Copy link
Contributor

@jmrodri jmrodri May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I often do this especially to address review comments.

Copy link
Contributor Author

@bparees bparees May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah so that's fine to do pre-merge, but a PR shouldn't be LGTMed while still in that state.

Copy link
Member

@ashcrow ashcrow May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixups + git rebase -i [...] is your friend when doing this on multiple separate commits. End result is squashed, targeted commits 👍

** use `git commit --fixup` and `git rebase -i --autosqash ...` to squash when you're getting ready to submit the PR

## Git Commit text

__First line:__ An english language sentence with capitalization, no period, max 50 characters
Copy link
Member

@ashcrow ashcrow May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that area of modification is quite helpful. Here are some PR title examples from MCO:

This is very helpful when going through history as well as when reviewing changes. Directly at a glance each PR's focus is quickly understood. On top of that, using this pattern helps folks avoid the all-the-things commits unless it explicitly must be done (say -- refactoring variable names, etc.. though one could say the area would be refactor: 😄)

Please note, these examples do end up showing functionality rather than the problem a user sees. Combining "describe the problem" with "area of modification" would be quite a nice clarification overall!

Copy link
Contributor Author

@bparees bparees May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting this for the PR title or the commit text? Given the commit text ends up in the release changelog i think it's undesirable in the commit text (first line).

having it in the PR title, or in the git text body seems ok, though it very much feels like a "ymmv" thing for how useful it is for different teams so i'm a little hesitant to put it in here as anything more than a "something to consider if you like it" suggestion.

Copy link
Member

@ashcrow ashcrow May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit text (first line). I can accept your reasoning against it though @bparees. Generally I think the area pattern is helpful, but if things are flowing into the changelog and could be a bit awkward it would be understandable to note specify the area of change that way. It may be helpful in the text body, but at that level I'd agree it's more of a "something to consider if you like it" at best.

Copy link
Member

@derekwaynecarr derekwaynecarr Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the area pattern is heavily used in installer if i recall.

/cc @abhinavdahiya should give his perspective as well.


__Additional information:__ Why was this change needed? What is the gist of the change?

* Almost every commit should have more than just a single line descriptor. If you are making a commit with no additional information, you should think twice about whether you've really provided enough information that another developer can understand the intent of the commit.
* Wrap lines at 72 characters, barring exceptions for links or other content that can't be wrapped.
* If the commit fixes a bug, describe in detail the problem that was observed that led up to this change
* Describe alternatives considered and why this change is the best one to make now, if applicable
* Add all relevant references to the commit text, including other commits, bugs, PRs, or other links that help explain or understand the change. Note however that you should minimize amending a commit with github references, as it will generate a lot of noise on reference issues or PRs. Instead, consider adding commits while iterating on a PR and squashing them once before merging.
* Consider including a tl;dr summary at the end of the commit text

[Canonical commit text example](https://github.com/openshift/origin/commit/d7a7ff790f87ae14ad106f2256ebafc1e7c91949)