> On 7. 7. 2022, at 19:28, Nir Soffer <nsoffer@redhat.com> wrote:
>
> On Wed, Jun 15, 2022 at 12:26 PM Yedidyah Bar David <didi@redhat.com> wrote:
>>
>> Hi all,
>>
>> I was annoyed for some time now by the fact that when I used some
>> github-CI-generated RPMs, with a git hash in their names, I could
>> never find this git hash anywhere - not in my local git repo, nor in
>> github. Why is it so? Because, if I got it right, the default for
>> 'actions/checkout@v2' is to merge the PR HEAD with the branch HEAD.
>> See e.g. [1]:
>>
>> HEAD is now at 7bbb40c9a Merge
>> 026bb9c672bf46786dd6d16f4cbe0ecfa84c531d into
>> 35e217936b5571e9657946b47333a563373047bb
>>
>> Meaning: my patch was 026bb9c, master was 35e2179, and the generated
>> RPMs will have 7bbb40c9a, not to be found anywhere else. If you check
>> the main PR page [3], you can find there '026bb9c', but not
>> '7bbb40c9a'.
>>
>> (Even 026bb9c might require some effort, e.g. "didib force-pushed the
>> add-hook-log-console branch 2 times, most recently from c90e658 to
>> 66ebc88 yesterday". I guess this is the result of github discouraging
>> force-pushes, in direct opposite of gerrit, which had a notion of
>> different patchsets for a single change. I already ranted about this
>> in the past, but that's not the subject of the current message).
>>
>> This is not just an annoyance, it's a real difference in semantics. In
>> gerrit/jenkins days, IIRC most/all projects I worked on, ran CI
>> testing/building on the pushed HEAD, and didn't touch it. Rebase, if
>> at all, happened either explicitly, or at merge time.
>
> I don't think that the action *rebases* the pr, it uses a merge commit
> but this adds newer commits on master on top of the pr, which may
> conflict or change the semantics of the pr.
>
>> actions/checkout's default, to auto-merge, is probably meant to be
>> more "careful" - to test what would happen if the code is merged. I
>> agree this makes sense. But I personally think it's almost always ok
>> to test on the pushed HEAD and not rebase/merge _implicitely_.
>>
>> What do you think?
>
> I agree, this is unexpected and unwanted behavior in particular for
> projects that disable merge commits (e.g. vdsm).
merge commits are disabled for all oVirt projects as per https://www.ovirt.org/develop/developer-guide/migrating_to_github.html
>
>> It should be easy to change, using [2]:
>>
>> - uses: actions/checkout@v2
>> with:
>> ref: ${{ github.event.pull_request.head.sha }}
we can really just create a trivial wrapper and replace globally with e.g.
- uses: ovirt/checkout
+1
As this needs to be included in each project separately, then I'd say let's minimize available options to ensure maximum consistency across all oVirt projects
1. I don't know how, and would have to learn quite a bit of github, to do this. That's the main reason I neglected this in my TODO folder and didn't reply yet. Perhaps someone already did something similar and would like to take over?