[ovirt-devel] Re: github testing: merge with branch, or use PR HEAD?

On Thu, Jul 21, 2022 at 9:35 AM Michal Skrivanek <mskrivan(a)redhat.com&gt; wrote: > > > > On 21. 7. 2022, at 9:09, Yedidyah Bar David <didi(a)redhat.com&gt; wrote: > > On Fri, Jul 8, 2022 at 11:30 AM Martin Perina <mperina(a)redhat.com&gt; wrote: >> >> >> >> On Fri, Jul 8, 2022 at 10:27 AM Michal Skrivanek <mskrivan(a)redhat.com&gt; wrote: >>> >>> >>> >>> > On 7. 7. 2022, at 19:28, Nir Soffer <nsoffer(a)redhat.com&gt; wrote: >>> > >>> > On Wed, Jun 15, 2022 at 12:26 PM Yedidyah Bar David <didi(a)redhat.com&gt; 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.

> > > 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