[ovirt-devel] Re: github testing: merge with branch, or use PR HEAD?
On Wed, Jun 15, 2022 at 12:26 PM Yedidyah Bar David 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@ovirt.org To unsubscribe send an email to devel-le...@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/WZ3W6BII34CTQXXLBYJB6W6ECCWEGM4J/
[ovirt-devel] Re: github testing: merge with branch, or use PR HEAD?
On Thu, Jul 7, 2022 at 1:31 PM Strahil Nikolov wrote: > > Erm ... does this mean that based on rpm, we can easily find the commit ? Exactly. Please note that this is a devel list, and we discuss dev builds... Release builds should always have nice and clean version numbers always pointing at relevant git tags. Best regards, > > Best Regards, > Strahil Nikolov > > On Thu, Jul 7, 2022 at 11:36, Yedidyah Bar David > 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 wrote: > > > > On Thu, Jun 16, 2022 at 1:27 PM Yedidyah Bar David wrote: > > > > > > On Wed, Jun 15, 2022 at 8:31 PM Michal Skrivanek > > > wrote: > > > > > > > > > > > > > > > > > On 15. 6. 2022, at 11:25, Yedidyah Bar David 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-ra...@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 maintenacewe 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 > > > > activeor 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@ovirt.org > To
[ovirt-devel] Re: github testing: merge with branch, or use PR HEAD?
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 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 wrote: > > On Thu, Jun 16, 2022 at 1:27 PM Yedidyah Bar David wrote: > > > > On Wed, Jun 15, 2022 at 8:31 PM Michal Skrivanek > > wrote: > > > > > > > > > > > > > On 15. 6. 2022, at 11:25, Yedidyah Bar David 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-ra...@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 maintenacewe 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 activeor > > > 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@ovirt.org To unsubscribe send an email to devel-le...@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/JKI57UOHTMQE3GWBAYQHZ4LTZ5PKLLI3/ ___ Devel mailing list -- devel@ovirt.org To unsubscribe send an email to devel-le...@ovirt.org Privacy Statement:
[ovirt-devel] Re: github testing: merge with branch, or use PR HEAD?
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 wrote: > > On Thu, Jun 16, 2022 at 1:27 PM Yedidyah Bar David wrote: > > > > On Wed, Jun 15, 2022 at 8:31 PM Michal Skrivanek > > wrote: > > > > > > > > > > > > > On 15. 6. 2022, at 11:25, Yedidyah Bar David 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-ra...@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 maintenacewe 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 activeor > > > 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@ovirt.org To unsubscribe send an email to devel-le...@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/JKI57UOHTMQE3GWBAYQHZ4LTZ5PKLLI3/