On 11. 8. 2022, at 8:24, Yedidyah Bar David <didi@redhat.com> wrote:

On Thu, Jul 21, 2022 at 5:04 PM Scott Dickerson <sdickers@redhat.com> wrote:



On Thu, Jul 21, 2022 at 9:35 AM Michal Skrivanek <mskrivan@redhat.com> wrote:



On 21. 7. 2022, at 9:09, Yedidyah Bar David <didi@redhat.com> wrote:

On Fri, Jul 8, 2022 at 11:30 AM Martin Perina <mperina@redhat.com> wrote:



On Fri, Jul 8, 2022 at 10:27 AM Michal Skrivanek <mskrivan@redhat.com> wrote:



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?


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.

https://github.com/oVirt/checkout-action
you have Write permissions 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/3903152852dc8a9d44484cbdc5c80de45774f090/automation/build.sh#L23-L33


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