On Wed, Aug 17, 2022 at 3:33 PM Yedidyah Bar David <[email protected]> wrote:
> On Tue, Aug 16, 2022 at 11:30 AM Michal Skrivanek <[email protected]> > wrote: > >> >> >> On 11. 8. 2022, at 8:24, Yedidyah Bar David <[email protected]> wrote: >> >> On Thu, Jul 21, 2022 at 5:04 PM Scott Dickerson <[email protected]> >> wrote: >> >> >> >> >> On Thu, Jul 21, 2022 at 9:35 AM Michal Skrivanek <[email protected]> >> wrote: >> >> >> >> >> On 21. 7. 2022, at 9:09, Yedidyah Bar David <[email protected]> wrote: >> >> On Fri, Jul 8, 2022 at 11:30 AM Martin Perina <[email protected]> wrote: >> >> >> >> >> On Fri, Jul 8, 2022 at 10:27 AM Michal Skrivanek <[email protected]> >> wrote: >> >> >> >> >> On 7. 7. 2022, at 19:28, Nir Soffer <[email protected]> wrote: >> >> On Wed, Jun 15, 2022 at 12:26 PM Yedidyah Bar David <[email protected]> >> 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 >> > > Pushed and merged a single PR [1], updated my test repo to use it [2], > tested it [3], seems ok. > > [1] https://github.com/oVirt/checkout-action/pull/1 > [2] > https://github.com/didib/test-checkout/commit/6d188ae88b8a58bde16d6a537123be9b90c14e0b > [3] https://github.com/didib/test-checkout/pull/3 > Also pushed: https://github.com/oVirt/otopi/pull/30 https://github.com/oVirt/ovirt-engine/pull/595 > > >> >> >> 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 >> >> >> > > -- > Didi > -- Didi
_______________________________________________ Devel mailing list -- [email protected] To unsubscribe send an email to [email protected] Privacy Statement: https://www.ovirt.org/privacy-policy.html oVirt Code of Conduct: https://www.ovirt.org/community/about/community-guidelines/ List Archives: https://lists.ovirt.org/archives/list/[email protected]/message/ZSM6RARX6NGVPBMZD2LBA4IJLOVRLW5Z/
