New PR and commit guidelines #2752

ShellyKa13 · 2023-06-14T12:55:17Z

The new guidelines should help us improve
the review process by having clearer PRs
with better explanations and commits divided
better to smaller parts to tell the reviewer
a story.

This is my take on how the guidelines should be, you are welcome to suggest other guideline, suggest better wording etc..
Your opinion is welcomed!

Release note:

NONE

alromeros · 2023-06-14T13:28:50Z

CONTRIBUTING.md

@@ -76,6 +76,10 @@ Maintainers are:
 ### PR Checklist

 Before your PR can be merged it must meet the following criteria:
+* PR Desciption should be informative, explaining what is in it and a high level design in case of new feature


Nice! I'm glad we are addressing this issue and I think it's a great idea to update this document. That said, I think we should differentiate between merging criteria (need for documentation, unit tests, etc.) and PR quality guidelines.

I would add these in a separate section and avoid mixing them with the strict merge criteria. Maybe we could also add examples for each one of the fields (for example, I think #2709 would fit nicely as an example for this first point).

I didn't want to overload the lines but an example can be good.
Regarding putting it in a different section Im not sure cause I do think this should be a criteria before merging a PR not just a quality thing.

mhenriks · 2023-06-14T13:17:25Z

CONTRIBUTING.md

+* PR Desciption should be informative, explaining what is in it and a high level design in case of new feature
+* Commits should be split logically with a single clear purpose for each.
+* Commits title should be concise and clear while commit message should be as informative as possible.
+* New changes done (after review for example) should be squashed to relevant commit.


Until commits are squashed on mainline (and let's not discuss that here), I see no advantage to this. IMO it makes the reviewer's job harder to exactly find what was changed during the review process. Personally would rather have all changes in new commits.

edit: NOT squashed

It depends cause if a second reviewer is going over the PR after more changes were introduced then he will comment on the commit and then in the newer "updates" commit he might see that everything that he invested time on reviewing was already fix or that his review is already unrelevant

I'm sure if we go through our PR history the vast majority of PRs were reviewed by a single person. So I'm not sure it makes sense to make the submitters job harder in every case. I say we revisit this if we ever move from squashing main

I can see why this is not trivial... harder to review vs harder to make sense of later
but until we start not squashing I lean towards dropping this

I think as long as you are not the first reviewer, this is one of the most important things to change because if you have a lot of new "review updates" commits you simply can't review the PR commit by commit since it is redundant cause stuff you will comment on might be irrelevant in the new update commits.. and I believe when reviewing all changed files and not by commit (especially in big PRs) its not only harder to review but also a potential to miss possible bugs..
If the majority thinks we should drop it then I will, but I still thing the value of squashing is better then how a bit harder it is for the first reviewer to review.
Also you have the comment you commented on the place you asked for a change so you can find if what you asked for was changed accordingly or not so I dont think its so much harder..

If the majority thinks we should drop it then I will,

@ShellyKa13 the burden is on you to convince the majority. Not the other way around. This PR will rot until a significant number of active contributors come forward in support.

I support @ShellyKa13 on this point. This was a principal part of her arguments. The commit log of CDI should be clean and changing the same exact lines in multiple commits introduces noise and makes going back to look at individual commits useless. The thrust of this PR is to "Make git-log readable again".

I think you can compare changes between force-pushes using the github UI so even if a PR author squashes their review comment driven changes into the set of existing commits you can still see only what's been changed since the last round of review.

mhenriks · 2023-06-14T13:23:31Z

CONTRIBUTING.md

@@ -76,6 +76,10 @@ Maintainers are:
 ### PR Checklist

 Before your PR can be merged it must meet the following criteria:
+* PR Desciption should be informative, explaining what is in it and a high level design in case of new feature
+* Commits should be split logically with a single clear purpose for each.
+* Commits title should be concise and clear while commit message should be as informative as possible.


"As possible" is a pretty high bar, no?

you are welcome to suggest a new wording for this sentence :)

Add a period after informative. End of sentence.

I think we may be able to drop

Commits title should be concise and clear while commit message should be as informative as possible.

I think the idea will get communicated in a softer manner via the previous point

Maybe "the commit message should clearly and completely state the nature and purpose of the changes in the commit"

I think we may be able to drop

Commits title should be concise and clear while commit message should be as informative as possible.

I think the idea will get communicated in a softer manner via the previous point

I think there is a big difference between how one splits and organizes the commits (the 2nd point) to make sure the title and message of each commit is clear. I have examples in existing PRs for such difference

akalenyu · 2023-06-15T09:36:28Z

CONTRIBUTING.md

@@ -76,6 +76,10 @@ Maintainers are:
 ### PR Checklist

 Before your PR can be merged it must meet the following criteria:
+* PR Desciption should be informative, explaining what is in it and a high level design in case of new feature
+* Commits should be split logically with a single clear purpose for each.


Suggested change

* Commits should be split logically with a single clear purpose for each.

* Commits split logically with a clear purpose for each.

akalenyu · 2023-06-15T09:37:13Z

CONTRIBUTING.md

@@ -76,6 +76,10 @@ Maintainers are:
 ### PR Checklist

 Before your PR can be merged it must meet the following criteria:
+* PR Desciption should be informative, explaining what is in it and a high level design in case of new feature
+* Commits should be split logically with a single clear purpose for each.
+* Commits title should be concise and clear while commit message should be as informative as possible.


I think we may be able to drop

Commits title should be concise and clear while commit message should be as informative as possible.

I think the idea will get communicated in a softer manner via the previous point

akalenyu · 2023-06-15T09:41:02Z

CONTRIBUTING.md

@@ -76,6 +76,10 @@ Maintainers are:
 ### PR Checklist

 Before your PR can be merged it must meet the following criteria:
+* PR Desciption should be informative, explaining what is in it and a high level design in case of new feature


Suggested change

* PR Desciption should be informative, explaining what is in it and a high level design in case of new feature

* Informative PR Description alongside an overview of the changes, high-level design in case of new feature

If you don't choose to go forward with this, note there's a typo Desciption

akalenyu · 2023-06-15T09:43:21Z

CONTRIBUTING.md

+* PR Desciption should be informative, explaining what is in it and a high level design in case of new feature
+* Commits should be split logically with a single clear purpose for each.
+* Commits title should be concise and clear while commit message should be as informative as possible.
+* New changes done (after review for example) should be squashed to relevant commit.


I can see why this is not trivial... harder to review vs harder to make sense of later
but until we start not squashing I lean towards dropping this

The new guidlines should help us improve the review process by having clearer PRs with better explanations and commits divided better to smaller parts to tell the reviewer a story. Signed-off-by: Shelly Kagan <skagan@redhat.com>

kubevirt-bot · 2023-06-26T14:45:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign aglitke for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

arnongilboa · 2023-06-26T15:56:33Z

CONTRIBUTING.md

+* Informative PR Description alongside an overview of the changes and a high-level design in case of a new feature
+* Commits split logically with a clear purpose.
+* Commits title concise and clear while commit message informative stating the nature and purpose of the changes in the commit.


You may add

Currently our merge policy is squashing all PR commits, so consider:

Splitting to incremental small PRs instead of a full feature PR

Interactive rebase (git rebase -i) before PR is getting merged, so it won’t look like a list of useless titles

kubevirt-bot · 2023-09-24T20:23:31Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.