On 11. 8. 2022, at 8:24, Yedidyah Bar David <didi(a)redhat.com>
wrote:
On Thu, Jul 21, 2022 at 5:04 PM Scott Dickerson <sdickers(a)redhat.com> wrote:
>
>
>
> On Thu, Jul 21, 2022 at 9:35 AM Michal Skrivanek <mskrivan(a)redhat.com> wrote:
>>
>>
>>
>> On 21. 7. 2022, at 9:09, Yedidyah Bar David <didi(a)redhat.com> wrote:
>>
>> On Fri, Jul 8, 2022 at 11:30 AM Martin Perina <mperina(a)redhat.com> wrote:
>>>
>>>
>>>
>>> On Fri, Jul 8, 2022 at 10:27 AM Michal Skrivanek <mskrivan(a)redhat.com>
wrote:
>>>>
>>>>
>>>>
>>>>> On 7. 7. 2022, at 19:28, Nir Soffer <nsoffer(a)redhat.com>
wrote:
>>>>>
>>>>> On Wed, Jun 15, 2022 at 12:26 PM Yedidyah Bar David
<didi(a)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?
>>
>>
>> Take a look at
https://github.com/oVirt/upload-rpms-action
>> minus tests (I hope Janos is not looking)...that makes it a new repo, and
license, readme, and yaml file with that snippet. that's it.
I am hesitant about the value of this exercise, but with Martin's
encouragement decided to try, and it seems to work indeed:
https://github.com/didib/checkout-head-sha
https://github.com/didib/test-checkout/pull/2
Check the output of 'git log' in the check - it shows the PR hash.
So please create a repo (e.g. oVirt/checkout or whatever) and I'll
push a PR there.
Didn't add test code :-).
>>
>>
>> 2. I already pushed (2 weeks ago) and merged (yesterday) to otopi, [1], which
simply does the above.
>>
>> 3. Scott now pushed [2], to the engine, doing the same, and I agree with him. So
am going to merge it soon, unless there are objections. If eventually someone creates an
oVirt action for this, we can always update to use it.
>>
>
> And just to add a bit more fuel to the fire: back in the old days when jenkins was
running CI for ovirt-web-ui, there were more hoops to jump through to get the PR head
commit instead of the PR merge commit when running builds. My solution there, and that
still works with the github actions, is:
https://github.com/oVirt/ovirt-web-ui/blob/3903152852dc8a9d44484cbdc5c80d...
>
>>
>> Best regards,
>>
>> [1]
https://github.com/oVirt/otopi/pull/25
>>
>> [2]
https://github.com/oVirt/ovirt-engine/pull/543
I merged this yesterday, while starting writing my current reply but
before deciding to try the above :-). Can change later.
Best regards,
--
Didi