Add guidelines on PR + commit text #311

bparees · 2020-05-08T17:25:04Z

also create a guidelines directory and move the enhancement template
into it.

bparees · 2020-05-08T17:27:39Z

alvaroaleman · 2020-05-08T18:51:02Z

guidelines/commit_and_pr_text.md

+
+## Number of Commits
+
+Strive for a single commit per PR.


If we have this guideline - Why don't we simply make squash merging the global default (with the option to override via a label, as we already offer today)?

i don't think we want random commit text squashed together from 15 different commits. i'd prefer people actually think through their commit messages and commits.

Well, the commit body you get from github on squash merge is the PR description, then a list of commits with their title and body (if any). So the random commit text will be at the very end of the body and can be ignored if you don't care.

Squashmerging has the advantage that it allows ppl to do small and granular commits (which can be super useful during development) without cluttering the repos history.

Using the github PR description as body rather than a single commits body has IMHO a coupe of advantages as well:

For ppl that already write commit messages, nothing changes, because github will put the body into the pr description if its only one commit

Ppl that are used to write proper descriptions in Github but are not used to write commit bodies do not have to change their workflow

Reviewing the PR description is much easier than reviewing the commit body (You don't have to select the commit and yalayala/check the pr out locally).

I prefer squash commits too. I've configured many of my repos to be squash commit.

On the other hand, splitting your PR into commits make them easier to review in chunks. Also, we should add text around making sure that the commits are bisectable i.e. tests should pass at each commit.

Squashing is good for avoiding lots of fixup commits in the history, but it is perfectly reasonable for a PR to have a well-structured series of commits (with helpful commit messages) where each commit is a separate logical change.

For everyone commenting on this goal....It says "strive for" not "thou shalt", and is followed by a section titled "valid reasons to have multiple commits". This is not saying we can never have multiple commits. If you think there are additional reasons not covered by the existing list, please add suggestions to that list. If this is a phrasing issue i'm happy to accept suggestions on a different wording that makes it clear... the intent is "you should generally default to one commit per PR unless you fall into one of the exception categories"

I want to emphasize that in general these are guidelines not rules. No one is going to be enforcing them strictly, we're just trying to get back to a little more discipline on our PR/git hygiene and give guidance to people who may be less experienced in what a good pr/commit looks like.

jmrodri

I think I'm okay with these guidelines. Will they be enforced at all? If not, then +1 if we are planning on having automation I'd be against that, often that turns into a 'how can I appease the bot so I can get on with my work' scenario.

jmrodri · 2020-05-08T19:18:20Z

guidelines/commit_and_pr_text.md

+
+Strive for a single commit per PR.
+
+Minimal commits per PR make 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)


One thing I try to do is make my vendor commits separate from all other work in the PR. I find it incredibly difficult to review 14,000 vendor changes :)

covered by https://github.com/openshift/enhancements/pull/311/files#diff-8092445902fc4142135544e8ba663eb1R35 no?

Minimal commits per PR make it easier to review the PR, [...], 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)

When I read this, I can't help but think there are 2 conflicting things here,

minimal commits

minimizing the number of commits per PR

The 1st item indeed helps with reviews, git blame, ... 2) conflicts with that in that it will not encourage people to split their work meaningfully since the recommendations push to use only 1 commit per PR.

In my experience, the smaller a commit is, the easier it is to review. If you use git blame because you want to understand why a change was made, the smaller the commit is, the more specific the commit message will be as to why the change was made. If you use git bisect to track down a bug on code you are not much familiar with, you also want to end up on a commit making as little changes as possible. Except for trivial changes/bug fixes, it's usually possible to split a change in several meaning full commits.

Actually, looking at the commit log of the 'canonical example' below ( openshift/origin@d7a7ff7 ), I'm under the impression each paragraph could have deserved its own commit, and this would have made it easier to assess (for example) whether "Remove the 'break' from the wait-loop's ClusterOperator iteration. The previous logic would only list the first unavailable operator. With this commit we now list each unavailable operator while we wait." does not introduce unwanted/unexpected logic changes. As it is now, the change is drowned in other unrelated changes, making the review process harder, and making a potential revert of just this part harder too.

I'm changing the text to Minimize your number of commits per PR with a goal of a single commit in many cases.

(again with the reasons and exceptions listed below that text).

The 1st item indeed helps with reviews, git blame, ... 2) conflicts with that in that it will not encourage people to split their work meaningfully since the recommendations push to use only 1 commit per PR.

Minimal number of commits helps with PR review imho, mainly because if you've got a PR with a vendor commit and then 50 other commits, I can't easily exclude the vendor commit from my review (I can either view all changes, or i have to go through each commit by itself). With a vendor commit plus one or two commits, I can easily review the one or two commits that aren't the vendor commit. That was the genesis of the statement that "minimal commits per PR make it easier to review the PR".

Certainly to some extent this probably depends on the nature of the PR as well as how reviewers prefer to operate.

Minimal sized commits (which weren't really discussed in this doc, i will rephrase "minimal commits to minimal numbers of commits, in the text) wasn't intended as a goal of this doc. Commits should be as big or as small as they need to be do to the related/cohesive task they are intended for. Sometimes that's 1 line in 1 file and sometimes that's 1000 lines in 1000 files (think utility function refactoring).

conflicts with that in that it will not encourage people to split their work meaningfully since the recommendations push to use only 1 commit per PR.

I think this is where the "consider if it should have been multiple PRs" statement comes in.

Actually, looking at the commit log of the 'canonical example' below ( openshift/origin@d7a7ff7 ), I'm under the impression each paragraph could have deserved its own commit,

point taken, the intent was that this was a good example of commit text (explaining what was done), not necessarily a good example of how to break down a body of work into specific commits. I will make that clear.

Actually, looking at the commit log of the 'canonical example' below ( openshift/origin@d7a7ff7 ), I'm under the impression each paragraph could have deserved its own commit,

point taken, the intent was that this was a good example of commit text (explaining what was done), not necessarily a good example of how to break down a body of work into specific commits. I will make that clear.

For what it's worth, it was clear that it was only a specific example in the context of 'good commit text', it was just convenient to reuse this one as a counter example.

Minimal number of commits helps with PR review imho, mainly because if you've got a PR with a vendor commit and then 50 other commits, I can't easily exclude the vendor commit from my review (I can either view all changes, or i have to go through each commit by itself). With a vendor commit plus one or two commits, I can easily review the one or two commits that aren't the vendor commit. That was the genesis of the statement that "minimal commits per PR make it easier to review the PR".

Certainly to some extent this probably depends on the nature of the PR as well as how reviewers prefer to operate.

Hmm it seems we definitely operate very differently, I have a hard time reviewing efficiently/correctly commits doing multiple things which could have been separated in smaller logical commits :-/ But maybe your "50 commits" example would be 50 commits such as "adjust previous commit", "fix bug in 5th commit of PR", "remove debug code added in PR", ...? In which case, yes, I agree it would be complicated to review.

Minimal sized commits (which weren't really discussed in this doc, i will rephrase "minimal commits to minimal numbers of commits, in the text) wasn't intended as a goal of this doc.

Which I'm worried is going to be a disincentive to splitting commits. As the developer, I'd just read that git commit -a and then PR is recommended by this doc :) This imo makes the reviewers task more complicated.

But maybe your "50 commits" example would be 50 commits such as "adjust previous commit", "fix bug in 5th commit of PR", "remove debug code added in PR", ...? In which case, yes, I agree it would be complicated to review.

That is definitely the clearest example of the kind of commit behavior we want to avoid.

Which I'm worried is going to be a disincentive to splitting commits. As the developer, I'd just read that git commit -a and then PR is recommended by this doc :) This imo makes the reviewers task more complicated.

well again, the idea here is that should be your default mode of operation(and that before you create that second commit, you consider whether it should instead be a second PR), but that there are valid reasons to have multiple commits.

jmrodri · 2020-05-08T19:22:11Z

guidelines/commit_and_pr_text.md

@@ -0,0 +1,53 @@
+# 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, patch managers understand risk/importance, and when hunting for regressions, helps us better understand what is/isn’t likely to be a cause.


On the SDK team we recently went through this process of changelog. We used to have a single changelog file that would need to be edited for EACH PR. This caused a bunch of subsequent PRs to have merge conflicts as the file was updated.

We also thought about making the commit messages the source of truth. This proved to be quite annoying as it was a departure from our current workflow, meant much more diligence to the commit messages.

Our final solution was to create the changelog from a collection of fragments that were added to each PR. This allowed us to better describe the change for the customer/user in the changelog.

operator-framework/operator-sdk#2761

the changelog isn't a committed file in the repo, it's auto generated doc that's available w/ the release.

I think that the kubernetes PR template https://github.com/kubernetes/kubernetes/edit/master/.github/PULL_REQUEST_TEMPLATE.md (a form of which CRI-O has adopted https://github.com/cri-o/cri-o/blob/master/.github/PULL_REQUEST_TEMPLATE.md) works well for generating changelog. It includes a release-note section that can be parsed to generate a changelog. It helps when a PR has multiple commits. The whole template itself is very useful to a reviewer for understanding the motivation for a PR.

jmrodri · 2020-05-08T19:25:15Z

guidelines/commit_and_pr_text.md

+
+* 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.


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.

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

jmrodri · 2020-05-08T19:31:18Z

guidelines/commit_and_pr_text.md

+
+## Number of Commits
+
+Strive for a single commit per PR.


I prefer squash commits too. I've configured many of my repos to be squash commit.

jmrodri · 2020-05-08T19:31:53Z

guidelines/commit_and_pr_text.md

+Valid reasons to have multiple 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.


Okay thanks for calling this out. This makes sense to me.

jmrodri · 2020-05-08T19:34:00Z

guidelines/commit_and_pr_text.md

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


I often do this especially to address review comments.

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

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

guidelines/commit_and_pr_text.md

bparees · 2020-05-08T19:50:48Z

I think I'm okay with these guidelines. Will they be enforced at all? If not, then +1 if we are planning on having automation I'd be against that, often that turns into a 'how can I appease the bot so I can get on with my work' scenario.

no plans to enforce, just tips for the org to follow and something to point to when a reviewer sees a PR/commit that doesn't seem sufficient.

cfergeau · 2020-05-11T09:43:57Z

guidelines/commit_and_pr_text.md

+
+* Wrap lines at 72 characters, barring exceptions for links or other content that can't be wrapped.
+* Avoid referencing github issues/PRs if you are repeatedly amending the commit as it generates a lot of noise in the referenced issue/PR.
+


I don't know if it's worth insisting that the "Additional information:" part is very important, and not optional? I've seen too many one line commit logs in openshift repositories which are quite cryptic for the occasional reader of the code :(

github issue references in logs are very useful when using git blame or similar. I've been told that instead of amending a commit/force pushing, one can use --fixup and that github will squash all the fixups at merge time regardless of the merge settings on the branch (I haven't tried it yet...)

I don't know if it's worth insisting that the "Additional information:" part is very important, and not optional?

I can reword to make it a stronger request/suggestion, but I'd like to avoid absolute requirements/rules since:

right now we have no plans to enforce anyway

"there's always an exception"

Yup, of course there are always exceptions :) Fix 'requierd' typo in doc does not really need more than a shortlog

lilic · 2020-05-11T10:31:33Z

guidelines/commit_and_pr_text.md

+
+# PR Metadata
+
+__PR title:__ Bug NNNN: A short sentence with capitalization and no period or the Jira tag for the issue “Feature TEAM-XXX:”


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?

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?)

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

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?

bparees · 2020-05-11T12:14:16Z

Pushed a few tweaks (in a new commit that will be squashed before merge!) in response to some of the feedback so far.

russellb · 2020-05-11T13:37:28Z

guidelines/commit_and_pr_text.md

@@ -0,0 +1,56 @@
+# 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, patch managers understand risk/importance, and when hunting for regressions, helps us better understand what is/isn’t likely to be a cause.


I would add something about current and future team members trying to understand why things are they way they are, as well. The commit log has the history of what we know about what was changed and why.

russellb · 2020-05-11T13:39:44Z

guidelines/commit_and_pr_text.md

+  * Changelog strips anything between square brackets that has no spaces - i.e. [release-4.4] is stripped but [release 4.4] would not be
+
+
+__PR description:__  Additional context reviewers need to understand your PR


Maybe add:

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

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.

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.

russellb · 2020-05-11T13:44:32Z

guidelines/commit_and_pr_text.md

+* 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.
+* Avoid referencing github issues/PRs if you are repeatedly amending the commit as it generates a lot of noise in the referenced issue/PR.
+


Some more ideas for this section:

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

russellb · 2020-05-11T13:47:02Z

guidelines/commit_and_pr_text.md

+
+* 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.
+* Avoid referencing github issues/PRs if you are repeatedly amending the commit as it generates a lot of noise in the referenced issue/PR.


I find references incredibly valuable, but this caveat does seem valuable. How about this:

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.

This also seems like "github best practices", maybe to be added up in the github section instead of here?

russellb · 2020-05-11T13:49:26Z

@bparees looks like you have some unrelated file renames in the PR

bparees · 2020-05-11T13:54:41Z

@bparees looks like you have some unrelated file renames in the PR

it was intentional(some minor but long overdue cleanup in this repo) but in the interest of practicing what i preach i will remove them.

abhinavdahiya · 2020-05-12T21:54:26Z

guidelines/commit_and_pr_text.md

+## Number of Commits
+
+Minimize your number of commits per PR with a goal of a single commit in many cases.


is it really the best indicator of better reviewable PR? if there are various commits with smaller logical changes, for such a PR is much easy to review instead of one commit with very large change.

personally I feel asking people to make one commit PRs would force people to put even large changes in one commit.

Maybe instead of number of commit we talk about non-vendor/non-generated changeset size?

is it really the best indicator of better reviewable PR?

It was not claimed as the best indicator of anything. it was positioned as a desirable attribute within the conditions/caveats discussed later.

if there are various commits with smaller logical changes, for such a PR is much easy to review instead of one commit with very large change.

there was discussion about this elsewhere in this PR, some people review commit by commit, others do it at a PR level. Which approach makes sense depends in part of how the PR is structured.

personally I feel asking people to make one commit PRs would force people to put even large changes in one commit.

I want to emphasize this as much as i can: there is no requirement to make single commit PRs. The ask is to think carefully about how many commits you are putting in a PR and why. Single commit is a goal(where applicable) and not a requirement.

Single commit is a goal(where applicable)

This is the concern, single commit is strongly recommended, so why bother with splitting for ease of maintainance/review/...?

This is the concern, single commit is strongly recommended, so why bother with splitting for ease of maintainance/review/...?

i'm going to reframe this statement to try to alleviate the concerns people are raising about it.

abhinavdahiya · 2020-05-12T21:57:18Z

guidelines/commit_and_pr_text.md

+
+Minimize your number of commits per PR with a goal of a single commit in many cases.
+
+Minimal numbers commits per PR make 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)


we don't squash commits on PR merge, so it is usually very easy to go from commit to the PR even if there were multiple commits in the PR using the github UI or git cli, and multiple commits are really not that problematic for tracking as long as they are focused... in my experience.

bparees · 2020-05-13T19:20:54Z

@cfergeau @abhinavdahiya @markmc @alvaroaleman i've pushed a new commit rephrasing the "multiple commits in a pr" guidance. ptal and see if it satisfies the concerns you've raised.

ashcrow

I've added a few suggestions. With or without those added this still gets folks closer to the same page on what an acceptable git commit and PR looks like. 👍.

ashcrow · 2020-05-18T15:50:34Z

guidelines/commit_and_pr_text.md

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


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

ashcrow · 2020-05-18T15:52:22Z

guidelines/commit_and_pr_text.md

+
+## Git Commit text
+
+__First line:__  An english language sentence with capitalization, no period, max 50 characters


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

cmd: Configure klog to match glog: cmd: Configure klog to match glog machine-config-operator#1730

pkg/daemon: Add IgnitionVersion to Daemon: pkg/daemon: Add IgnitionVersion to Daemon machine-config-operator#1729

mcs: Log User-Agent: mcs: Log User-Agent machine-config-operator#1705

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!

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.

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.

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

/cc @abhinavdahiya should give his perspective as well.

guidelines/commit_and_pr_text.md

bparees · 2020-05-18T17:15:48Z

@ashcrow comments replied to and/or addressed in a new commit for you.

ashcrow

Looks even better now 😄

sdodson · 2020-06-01T18:16:49Z

guidelines/commit_and_pr_text.md

+  * (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


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?

sdodson

lgtm

derekwaynecarr

this generally lgtm as general guidelines.
are there any guidelines we want to enforce via tooling?

derekwaynecarr · 2020-06-01T18:30:09Z

guidelines/commit_and_pr_text.md

+
+## Git Commit text
+
+__First line:__  An english language sentence with capitalization, no period, max 50 characters


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

/cc @abhinavdahiya should give his perspective as well.

bparees · 2020-06-01T18:59:34Z

are there any guidelines we want to enforce via tooling?

personally i'd rather not, there are always going to be "i just need to fix this typo" commits and i can't think of a minimal rule that's both:

going to allow people to easily fix minor issues w/o feeling frustrated about having to satisfy some bot requirement
going to provide value by enforcing some minimal standard/information content

to me this really is just an awareness thing that reviewers should help train PR submitters into doing properly.

ashcrow · 2020-06-01T19:03:11Z

I second @bparees notes above. This is a solid step and we can iterate with modifications, tooling, etc.. over time as appropriate.

derekwaynecarr · 2020-06-01T19:05:27Z

@bparees ok, given the above around enforcement, then this doc looks like great guidance.

/approve
/lgtm

openshift-ci-robot · 2020-06-01T19:05:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, bparees, derekwaynecarr, sdodson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ashcrow,bparees,derekwaynecarr,sdodson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from ironcladlou and sdodson May 8, 2020 17:25

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2020

openshift-ci-robot assigned derekwaynecarr and smarterclayton May 8, 2020

bparees force-pushed the guides branch from 40205f0 to d4e2270 Compare May 8, 2020 17:28

alvaroaleman reviewed May 8, 2020

View reviewed changes

jmrodri reviewed May 8, 2020

View reviewed changes

mrunalp reviewed May 8, 2020

View reviewed changes

guidelines/commit_and_pr_text.md Show resolved Hide resolved

bparees force-pushed the guides branch from d4e2270 to 6988ef2 Compare May 8, 2020 20:05

cfergeau reviewed May 11, 2020

View reviewed changes

lilic reviewed May 11, 2020

View reviewed changes

russellb reviewed May 11, 2020

View reviewed changes

bparees force-pushed the guides branch from 18f5742 to aa8f8fa Compare May 11, 2020 13:47

bparees force-pushed the guides branch from aa8f8fa to a2e9d3d Compare May 11, 2020 13:53

bparees force-pushed the guides branch from a2e9d3d to 4c36645 Compare May 11, 2020 14:02

abhinavdahiya reviewed May 12, 2020

View reviewed changes

ashcrow self-requested a review May 18, 2020 15:34

ashcrow approved these changes May 18, 2020

View reviewed changes

bparees force-pushed the guides branch from d6a7e74 to 65fbe7c Compare June 1, 2020 13:43

Add guidelines on PR + commit text

8ece958

bparees force-pushed the guides branch from 65fbe7c to 8ece958 Compare June 1, 2020 13:44

sdodson reviewed Jun 1, 2020

View reviewed changes

sdodson approved these changes Jun 1, 2020

View reviewed changes

derekwaynecarr reviewed Jun 1, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2020

openshift-merge-robot merged commit fca1f28 into openshift:master Jun 1, 2020

bparees deleted the guides branch August 19, 2020 13:04


		Strive for a single commit per PR.

		Minimal commits per PR make 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)

		@@ -0,0 +1,53 @@
		# 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, patch managers understand risk/importance, and when hunting for regressions, helps us better understand what is/isn’t likely to be a cause.


		* Wrap lines at 72 characters, barring exceptions for links or other content that can't be wrapped.
		* Avoid referencing github issues/PRs if you are repeatedly amending the commit as it generates a lot of noise in the referenced issue/PR.


		# PR Metadata

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

		@@ -0,0 +1,56 @@
		# 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, patch managers understand risk/importance, and when hunting for regressions, helps us better understand what is/isn’t likely to be a cause.

		* Changelog strips anything between square brackets that has no spaces - i.e. [release-4.4] is stripped but [release 4.4] would not be


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

		## Number of Commits

		Minimize your number of commits per PR with a goal of a single commit in many cases.


		Minimize your number of commits per PR with a goal of a single commit in many cases.

		Minimal numbers commits per PR make 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)


		## Git Commit text

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

Add guidelines on PR + commit text #311

Add guidelines on PR + commit text #311

Uh oh!

Conversation

bparees commented May 8, 2020

Uh oh!

bparees commented May 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfergeau May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bparees commented May 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfergeau May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp May 8, 2020 •

edited

Loading

cfergeau May 11, 2020 •

edited

Loading

cfergeau May 11, 2020 •

edited

Loading