[Gluster-devel] Pull Request review workflow

[Csaba Henk]

On Thu, Oct 15, 2020 at 1:01 PM Amar Tumballi <amar at kadalu.io> 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 <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.
>

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

https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/about-pull-request-merges#merge-message-for-a-squash-merge

).

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
> --
> https://kadalu.io
>
>
>> Regards,
>>
>> Xavi
>>
>
> Regards
Csaba
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20201019/33243e98/attachment.html>