> On 15. 6. 2022, at 11:25, 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? huh, I wondered about that same thing today.... Thank you for explaining why I couldn't find that hash anywhere > 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). We should create ovirt-github-rants(a)ovirt.org, I'd certainly contribute:-) It's amazing how horrible _and_ popular github is. > > 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. > > 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? > > It should be easy to change, using [2]: > > - uses: actions/checkout@v2 > with: > ref: ${{ github.event.pull_request.head.sha }} > > No need to reach a complete consensus - can be decided upon > per-project/repo. github is always quite horrible to maintain some consistency across projects...yeah, I'd really like to have the same approach for every single project, it simplifies the maintenace....we do have a lot of projects and many are not very active and they easily fall behind. After all we have 160 projects in oVirt org but only ~30 are active....or rather 30 are in use for oVirt compose and ~10 are active. +1 on using it everywhere we have our own action for rpms and buildcontainer for unified build environment (with a shameful exception of vdsm!)....it's probably overkill for checkout to use oVirt's action > But if you disagree, I'd like to understand why.

> Thanks. > > Best regards, > > [1] https://github.com/oVirt/vdsm/runs/6881311961?check_suite_focus=true > > [2] https://github.com/marketplace/actions/checkout?version=v2.4.2#checkout-p... > > [3] https://github.com/oVirt/vdsm/pull/249 > -- > Didi > _______________________________________________ > Devel mailing list -- devel(a)ovirt.org > To unsubscribe send an email to devel-leave(a)ovirt.org > 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/devel@ovirt.org/message/7SEFKOASOAT...

On Thu, Jun 16, 2022 at 1:27 PM Yedidyah Bar David <didi(a)redhat.com&gt; wrote: > > On Wed, Jun 15, 2022 at 8:31 PM Michal Skrivanek <mskrivan(a)redhat.com&gt; wrote: > > > > > > > > > On 15. 6. 2022, at 11:25, 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? > > > > huh, I wondered about that same thing today.... > > Thank you for explaining why I couldn't find that hash anywhere > > > > > 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). > > > > We should create ovirt-github-rants(a)ovirt.org, I'd certainly contribute:-) It's amazing how horrible _and_ popular github is. > > > > > > > > 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. > > > > > > 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? > > > > > > It should be easy to change, using [2]: > > > > > > - uses: actions/checkout@v2 > > > with: > > > ref: ${{ github.event.pull_request.head.sha }} > > > > > > No need to reach a complete consensus - can be decided upon > > > per-project/repo. > > > > github is always quite horrible to maintain some consistency across projects...yeah, I'd really like to have the same approach for every single project, it simplifies the maintenace....we do have a lot of projects and many are not very active and they easily fall behind. After all we have 160 projects in oVirt org but only ~30 are active....or rather 30 are in use for oVirt compose and ~10 are active. > > > > +1 on using it everywhere > > we have our own action for rpms and buildcontainer for unified build environment (with a shameful exception of vdsm!)....it's probably overkill for checkout to use oVirt's action

> > > > > But if you disagree, I'd like to understand why. > > If someone _wants_ the current behavior (merge to HEAD) but is just > annoyed by the commit hash, an alternative is to use the PR hash in > the name. Now tried this POC, seems to work: > > https://github.com/oVirt/otopi/pull/23 Also this one. Much simpler, but perhaps we still want the refactoring of ^^. https://github.com/oVirt/otopi/pull/24

Erm ... does this mean that based on rpm, we can easily find the commit ?

Best Regards, Strahil Nikolov On Thu, Jul 7, 2022 at 11:36, Yedidyah Bar David <didi(a)redhat.com&gt; wrote: 3 weeks later, no-one other than Michal replied... Adding a few more people trying to get their attention. On Thu, Jun 16, 2022 at 3:31 PM Yedidyah Bar David <didi(a)redhat.com&gt; wrote: > > On Thu, Jun 16, 2022 at 1:27 PM Yedidyah Bar David <didi(a)redhat.com&gt; wrote: > > > > On Wed, Jun 15, 2022 at 8:31 PM Michal Skrivanek <mskrivan(a)redhat.com&gt; wrote: > > > > > > > > > > > > > On 15. 6. 2022, at 11:25, 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? > > > > > > huh, I wondered about that same thing today.... > > > Thank you for explaining why I couldn't find that hash anywhere > > > > > > > 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). > > > > > > We should create ovirt-github-rants(a)ovirt.org, I'd certainly contribute:-) It's amazing how horrible _and_ popular github is. > > > > > > > > > > > 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. > > > > > > > > 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? > > > > > > > > It should be easy to change, using [2]: > > > > > > > > - uses: actions/checkout@v2 > > > > with: > > > > ref: ${{ github.event.pull_request.head.sha }} > > > > > > > > No need to reach a complete consensus - can be decided upon > > > > per-project/repo. > > > > > > github is always quite horrible to maintain some consistency across projects...yeah, I'd really like to have the same approach for every single project, it simplifies the maintenace....we do have a lot of projects and many are not very active and they easily fall behind. After all we have 160 projects in oVirt org but only ~30 are active....or rather 30 are in use for oVirt compose and ~10 are active. > > > > > > +1 on using it everywhere > > > we have our own action for rpms and buildcontainer for unified build environment (with a shameful exception of vdsm!)....it's probably overkill for checkout to use oVirt's action Yes, that's my own preference as well. I am going to push patches to a few projects doing this, perhaps including ones I am not very active in. Feel free to ignore/disagree and I'll close. > > > > > > > But if you disagree, I'd like to understand why. > > > > If someone _wants_ the current behavior (merge to HEAD) but is just > > annoyed by the commit hash, an alternative is to use the PR hash in > > the name. Now tried this POC, seems to work: > > > > https://github.com/oVirt/otopi/pull/23 > > Also this one. Much simpler, but perhaps we still want the refactoring of ^^. > > https://github.com/oVirt/otopi/pull/24 (Going to close both of the above) Best regards, -- Didi _______________________________________________ Devel mailing list -- devel(a)ovirt.org To unsubscribe send an email to devel-leave(a)ovirt.org 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/devel@ovirt.org/message/JKI57UOHTMQ...

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

> It should be easy to change, using [2]: > > - uses: actions/checkout@v2 > with: > ref: ${{ github.event.pull_request.head.sha }}

+1 Nir _______________________________________________ Devel mailing list -- devel(a)ovirt.org To unsubscribe send an email to devel-leave(a)ovirt.org 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/devel@ovirt.org/message/WZ3W6BII34C...

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 > > > > Nir > > _______________________________________________ > > Devel mailing list -- devel(a)ovirt.org > > To unsubscribe send an email to devel-leave(a)ovirt.org > > 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/devel@ovirt.org/message/WZ3W6BII34C... > _______________________________________________ > Devel mailing list -- devel(a)ovirt.org > To unsubscribe send an email to devel-leave(a)ovirt.org > 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/devel@ovirt.org/message/HLUUV2YMDGN... > -- Martin Perina Manager, Software Engineering Red Hat Czech s.r.o.

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.

Best regards, [1] https://github.com/oVirt/otopi/pull/25 [2] https://github.com/oVirt/ovirt-engine/pull/543 > >> > >> > +1 >> > >> > Nir >> > _______________________________________________ >> > Devel mailing list -- devel(a)ovirt.org >> > To unsubscribe send an email to devel-leave(a)ovirt.org >> > 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/devel@ovirt.org/message/WZ3W6BII34C... >> _______________________________________________ >> Devel mailing list -- devel(a)ovirt.org >> To unsubscribe send an email to devel-leave(a)ovirt.org >> 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/devel@ovirt.org/message/HLUUV2YMDGN... >> > > > -- > Martin Perina > Manager, Software Engineering > Red Hat Czech s.r.o. > -- Didi

On 11. 8. 2022, at 8:24, Yedidyah Bar David <didi(a)redhat.com&gt; wrote: On Thu, Jul 21, 2022 at 5:04 PM Scott Dickerson <sdickers(a)redhat.com&gt; wrote: > > > > 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. 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

On Tue, Aug 16, 2022 at 11:30 AM Michal Skrivanek <mskrivan(a)redhat.com&gt; wrote: > > > On 11. 8. 2022, at 8:24, Yedidyah Bar David <didi(a)redhat.com&gt; wrote: > > On Thu, Jul 21, 2022 at 5:04 PM Scott Dickerson <sdickers(a)redhat.com&gt; > wrote: > > > > > 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. > > > 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/6d188ae88b8a58bde16d6a53712... [3] https://github.com/didib/test-checkout/pull/3

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