Pull Request review workflow

Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Pull Request review workflow

Xavi Hernandez-2
Hi all,

after the recent switch to GitHub, I've seen that reviews that require multiple iterations are hard to follow using the old workflow we were using in Gerrit.

Till now we basically amended the commit and pushed it again. Gerrit had a feature to calculate diffs between versions of the patch, so it was relatively easy to follow the changes between iterations (unless there was a big change in the base branch and the patch was rebased).

In GitHub we don't have this feature (at least I haven't seen it). So I'm proposing to change this workflow.

The idea is to create a PR with the initial commit. When a modification needs to be done as a result of the review, instead of amending the existing commit, we should create a new commit. From the review tool in GitHub it's very easy to check individual commits.

Once the review is finished, the patch will be merged with the "Squash and Merge" option, that will combine all the commits into a single one before merging, so the end result will be exactly the same we had with Gerrit.

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.

What do you think ?

Regards,

Xavi

_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel

Reply | Threaded
Open this post in threaded view
|

Re: Pull Request review workflow

Amar Tumballi-3
Thanks for taking time on this, and sending this note Xavi!

Some comments inline!

On Thu, Oct 15, 2020 at 4:03 PM Xavi Hernandez <[hidden email]> wrote:
Hi all,

after the recent switch to GitHub, I've seen that reviews that require multiple iterations are hard to follow using the old workflow we were using in Gerrit.

Till now we basically amended the commit and pushed it again. Gerrit had a feature to calculate diffs between versions of the patch, so it was relatively easy to follow the changes between iterations (unless there was a big change in the base branch and the patch was rebased).

In GitHub we don't have this feature (at least I haven't seen it). So I'm proposing to change this workflow.

The idea is to create a PR with the initial commit. When a modification needs to be done as a result of the review, instead of amending the existing commit, we should create a new commit. From the review tool in GitHub it's very easy to check individual commits.


+1
 
Once the review is finished, the patch will be merged with the "Squash and Merge" option, that will combine all the commits into a single one before merging, so the end result will be exactly the same we had with Gerrit.


+1 
Just a note to the maintainers who are merging PRs to have patience and check the commit message when there are more than 1 commits in PR.
 

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.


With github workflow, we don't need './rfc.sh' in my personal opinion. I ported it to new branch and github considering the number of developers who are used to it.  If you do the changes as per github, then you would have a separate branch per PR (ie, feature/bug), so you are at your own to decide when to rebase.

What do you think ?


I agree, we can remove -f option of ./rfc.sh and also the rebase part in ./rfc.sh!
 
Regards,
Amar
--
 
Regards,

Xavi


_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel

Reply | Threaded
Open this post in threaded view
|

Re: Pull Request review workflow

spamecha
In reply to this post by Xavi Hernandez-2
+1 for this. I too face the issue with losing old changes once a new commit is pushed.

Regards,
Sheetal Pamecha


On Thu, Oct 15, 2020 at 4:03 PM Xavi Hernandez <[hidden email]> wrote:
Hi all,

after the recent switch to GitHub, I've seen that reviews that require multiple iterations are hard to follow using the old workflow we were using in Gerrit.

Till now we basically amended the commit and pushed it again. Gerrit had a feature to calculate diffs between versions of the patch, so it was relatively easy to follow the changes between iterations (unless there was a big change in the base branch and the patch was rebased).

In GitHub we don't have this feature (at least I haven't seen it). So I'm proposing to change this workflow.

The idea is to create a PR with the initial commit. When a modification needs to be done as a result of the review, instead of amending the existing commit, we should create a new commit. From the review tool in GitHub it's very easy to check individual commits.

Once the review is finished, the patch will be merged with the "Squash and Merge" option, that will combine all the commits into a single one before merging, so the end result will be exactly the same we had with Gerrit.

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.

What do you think ?

Regards,

Xavi
_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel


_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel

Reply | Threaded
Open this post in threaded view
|

Re: Pull Request review workflow

spamecha
In reply to this post by Amar Tumballi-3

Regards,
Sheetal Pamecha


On Thu, Oct 15, 2020 at 4:31 PM Amar Tumballi <[hidden email]> wrote:
Thanks for taking time on this, and sending this note Xavi!

Some comments inline!

On Thu, Oct 15, 2020 at 4:03 PM Xavi Hernandez <[hidden email]> wrote:
Hi all,

after the recent switch to GitHub, I've seen that reviews that require multiple iterations are hard to follow using the old workflow we were using in Gerrit.

Till now we basically amended the commit and pushed it again. Gerrit had a feature to calculate diffs between versions of the patch, so it was relatively easy to follow the changes between iterations (unless there was a big change in the base branch and the patch was rebased).

In GitHub we don't have this feature (at least I haven't seen it). So I'm proposing to change this workflow.

The idea is to create a PR with the initial commit. When a modification needs to be done as a result of the review, instead of amending the existing commit, we should create a new commit. From the review tool in GitHub it's very easy to check individual commits.


+1
 
Once the review is finished, the patch will be merged with the "Squash and Merge" option, that will combine all the commits into a single one before merging, so the end result will be exactly the same we had with Gerrit.


+1 
Just a note to the maintainers who are merging PRs to have patience and check the commit message when there are more than 1 commits in PR.
 

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.


With github workflow, we don't need './rfc.sh' in my personal opinion. I ported it to new branch and github considering the number of developers who are used to it.  If you do the changes as per github, then you would have a separate branch per PR (ie, feature/bug), so you are at your own to decide when to rebase.

Actually True, Since the migration to github. I have not been using ./rfc.sh and For me it's easier and cleaner.
 
What do you think ?


I agree, we can remove -f option of ./rfc.sh and also the rebase part in ./rfc.sh!
 
Regards,
Amar
--
 
Regards,

Xavi

_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel


_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel

Reply | Threaded
Open this post in threaded view
|

Re: Pull Request review workflow

Ravishankar N


On 15/10/20 4:36 pm, Sheetal Pamecha wrote:

+1 
Just a note to the maintainers who are merging PRs to have patience and check the commit message when there are more than 1 commits in PR.

Makes sense.

 

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.



I think we would also need to rebase if say some .t failure was fixed and we need to submit the PR on top of that, unless "run regression" always applies your PR on the latest HEAD in the concerned branch and triggers the regression.



Actually True, Since the migration to github. I have not been using ./rfc.sh and For me it's easier and cleaner.


Me as well :)

-Ravi

_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel

Reply | Threaded
Open this post in threaded view
|

Re: Pull Request review workflow

Xavi Hernandez
Hi Ravi,

On Thu, Oct 15, 2020 at 1:27 PM Ravishankar N <[hidden email]> wrote:


On 15/10/20 4:36 pm, Sheetal Pamecha wrote:

+1 
Just a note to the maintainers who are merging PRs to have patience and check the commit message when there are more than 1 commits in PR.

Makes sense.

 

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.



I think we would also need to rebase if say some .t failure was fixed and we need to submit the PR on top of that, unless "run regression" always applies your PR on the latest HEAD in the concerned branch and triggers the regression.


Yes, I agree that sometimes we need a rebase, but I would do that only if necessary by running a manual 'git rebase'.

I don't think we can do an automatic rebase before running a regression, because there could be conflicts that cannot be fixed automatically.

Xavi



Actually True, Since the migration to github. I have not been using ./rfc.sh and For me it's easier and cleaner.


Me as well :)

-Ravi
_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel


_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel

Reply | Threaded
Open this post in threaded view
|

Re: Pull Request review workflow

Xavi Hernandez
In reply to this post by Ravishankar N
If everyone agrees, I'll prepare a PR with the changes in rfc.sh and documentation to implement this change.

Xavi

On Thu, Oct 15, 2020 at 1:27 PM Ravishankar N <[hidden email]> wrote:


On 15/10/20 4:36 pm, Sheetal Pamecha wrote:

+1 
Just a note to the maintainers who are merging PRs to have patience and check the commit message when there are more than 1 commits in PR.

Makes sense.

 

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.



I think we would also need to rebase if say some .t failure was fixed and we need to submit the PR on top of that, unless "run regression" always applies your PR on the latest HEAD in the concerned branch and triggers the regression.



Actually True, Since the migration to github. I have not been using ./rfc.sh and For me it's easier and cleaner.


Me as well :)

-Ravi
_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel


_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel

Reply | Threaded
Open this post in threaded view
|

Re: Pull Request review workflow

Ashish Pandey

I think it's a very good suggestion, I have faced this issue too.
I think we should it now before we get used to of the current process :)

---
Ashish



From: "Xavi Hernandez" <[hidden email]>
To: "gluster-devel" <[hidden email]>
Sent: Thursday, October 15, 2020 6:16:06 PM
Subject: Re: [Gluster-devel] Pull Request review workflow

If everyone agrees, I'll prepare a PR with the changes in rfc.sh and documentation to implement this change.

Xavi

On Thu, Oct 15, 2020 at 1:27 PM Ravishankar N <[hidden email]> wrote:


On 15/10/20 4:36 pm, Sheetal Pamecha wrote:

+1 
Just a note to the maintainers who are merging PRs to have patience and check the commit message when there are more than 1 commits in PR.

Makes sense.

 

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.



I think we would also need to rebase if say some .t failure was fixed and we need to submit the PR on top of that, unless "run regression" always applies your PR on the latest HEAD in the concerned branch and triggers the regression.



Actually True, Since the migration to github. I have not been using ./rfc.sh and For me it's easier and cleaner.


Me as well :)

-Ravi
_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel


_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel



_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel

Reply | Threaded
Open this post in threaded view
|

Re: Pull Request review workflow

Csaba Henk-3
In reply to this post by Amar Tumballi-3


On Thu, Oct 15, 2020 at 1:01 PM Amar Tumballi <[hidden email]> wrote:
Thanks for taking time on this, and sending this note Xavi!

Some comments inline!

On Thu, Oct 15, 2020 at 4:03 PM Xavi Hernandez <[hidden email]> wrote:
Hi all,

after the recent switch to GitHub, I've seen that reviews that require multiple iterations are hard to follow using the old workflow we were using in Gerrit.

Till now we basically amended the commit and pushed it again. Gerrit had a feature to calculate diffs between versions of the patch, so it was relatively easy to follow the changes between iterations (unless there was a big change in the base branch and the patch was rebased).

In GitHub we don't have this feature (at least I haven't seen it). So I'm proposing to change this workflow.

The idea is to create a PR with the initial commit. When a modification needs to be done as a result of the review, instead of amending the existing commit, we should create a new commit. From the review tool in GitHub it's very easy to check individual commits.


+1
 
Once the review is finished, the patch will be merged with the "Squash and Merge" option, that will combine all the commits into a single one before merging, so the end result will be exactly the same we had with Gerrit.


+1 
Just a note to the maintainers who are merging PRs to have patience and check the commit message when there are more than 1 commits in PR.

Well,  just to _check_ it by the maintainer won't suffice; when there are several commits are in the PR, Github's prefilled default message just consists of a list of the individual commits, so the maintainer will have to actively edit the commit message to restore the original one. (See


).

And that's unambiguous only if the commit message is not intended to change throughout the amendments.

What should be the protocol when the developer wants to update the commit message? With the force push single commit approach, the commit message gets updated and reused for the upstream commit too (see also above link), so there is nowhere to get it wrong. For multi-commit pr, there should be specific instructions for commit message editing to not get it wrong.

 
 

Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn't do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.


With github workflow, we don't need './rfc.sh' in my personal opinion. I ported it to new branch and github considering the number of developers who are used to it.  If you do the changes as per github, then you would have a separate branch per PR (ie, feature/bug), so you are at your own to decide when to rebase.

What do you think ?


I agree, we can remove -f option of ./rfc.sh and also the rebase part in ./rfc.sh!
 
Regards,
Amar
--
 
Regards,

Xavi

Regards
Csaba

_______________________________________________

Community Meeting Calendar:

Schedule -
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
Bridge: https://bluejeans.com/441850968




Gluster-devel mailing list
[hidden email]
https://lists.gluster.org/mailman/listinfo/gluster-devel