[Gluster-devel] Pull Request review workflow

[Amar Tumballi]

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 <xhernandez at redhat.com>
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
--
https://kadalu.io


> Regards,
>
> Xavi
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20201015/f60acc21/attachment-0001.html>