Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CI:BUILD] Packit: Copr rpm version sha #20091

Merged
merged 1 commit into from
Sep 25, 2023
Merged

[CI:BUILD] Packit: Copr rpm version sha #20091

merged 1 commit into from
Sep 25, 2023

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Sep 21, 2023

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 21, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023
TomSweeneyRedHat
TomSweeneyRedHat reviewed Sep 21, 2023
View reviewed changes
.packit.yaml Outdated
@@ -16,8 +16,10 @@ actions:
jobs:
- job: copr_build
trigger: pull_request
notifications:
failure_comment:
message: "Ephemeral COPR build failed. @lsm5 please check."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine for a draft, but please remove lsm for final.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomSweeneyRedHat that's from #20061 . It's intentional.

@packit-as-a-service
Copy link

Cockpit tests failed for commit 226507c02a66a0ca2a60338963983c729a155d1f. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

The f39/revdeps failure is unrelated, and a rare test-only race condition; I'll put it on my todo list. Feel free to retry or ignore.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2023
@lsm5 lsm5 force-pushed the copr-rpm-version-sha branch from 8e3f5d6 to 668d957 Compare September 22, 2023 14:15
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2023
@lsm5
[CI:BUILD] Packit: show SHORT_SHA in podman --version for COPR builds
e658749 
This change will allow `podman --version` from COPR rpm builds to be of
the form `X.Y.Z-dev-SHORT_SHA`.

Also specify a `copr_build` macro as it feels more intuitive
than `copr_username` or `copr_project`.

[NO NEW TESTS NEEDED]

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 force-pushed the copr-rpm-version-sha branch from 668d957 to e658749 Compare September 22, 2023 14:19
@packit-as-a-service
Copy link

Cockpit tests failed for commit 8e3f5d6513a5ef69d9edaaaeac4e8c65e2097dec. @martinpitt, @jelly, @mvollmer please check.

@lsm5 lsm5 marked this pull request as ready for review September 22, 2023 15:06
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2023
@lsm5
Copy link
Member Author

lsm5 commented Sep 22, 2023

@cevich @benoitf The rpm installed from https://download.copr.fedorainfracloud.org/results/packit/containers-podman-20091/fedora-rawhide-x86_64/06433584-podman/ built in packit tasks shows:

$ podman --version
podman version 4.7.0-dev-e65874903

This is only enabled for copr builds in spec and won't affect official Fedora builds.

So, this PR is good to go. PTAL

@benoitf
Copy link
Contributor

benoitf commented Sep 22, 2023

nice 👍

TomSweeneyRedHat
TomSweeneyRedHat reviewed Sep 22, 2023
View reviewed changes
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lsm5
Copy link
Member Author

lsm5 commented Sep 22, 2023

@containers/podman-maintainers good to merge. PTAL

cevich
cevich reviewed Sep 22, 2023
View reviewed changes
rpm/podman.spec
# podman --version should show short sha
sed -i "s/^const RawVersion = .*/const RawVersion = \"##VERSION##-##SHORT_SHA##\"/" version/rawversion/version.go
# use ParseTolerant to allow short sha in version
sed -i "s/^var Version.*/var Version, err = semver.ParseTolerant(rawversion.RawVersion)/" version/version.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion: I had one of these "check the go source" version before, and it got annoying to fix every time something in go-land changed. That's why I made the test/version/version Makefile target. Run that, and your sed-ing can probably be swapped for a printf or an echo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarification:

$ make test/version/version
$ test/version/version
4.7.0-dev

Copy link
Member Author

@lsm5 lsm5 Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is semver.MustParse . 4.8.0-dev-SHORT_SHA isn't strictly semver compliant. If I change the version in version/rawversion/version.go to 4.8.0-dev-foobar , running ./test/version/version will still give me 4.8.0-dev and not 4.8.0-dev-foobar.

Let me know if I misread and you meant something else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, no, I read your sed command wrong. This is changing version.go which seems wors-er (IMHO). The issue is, sed will happily do nothing if no match is found. It's also trivial for someone or some tool to modify version.go and break sed in a different or more silent way.

Perhaps it would be a slight improvement if you had a kind of "patch-template"? Render that, verify it somehow, then apply it with patch (or the spec-file magic equivalent). That way it might be more likely to fail-hard on a breaking code change? I dunno, maybe there's a better/more golang or Makefile idiomatic way to override the version value in the code.

I bet @edsantiago or @vrothberg might have some ideas here, they're more inventive and know the toolchain much better than I.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm open to suggestions, but this is copr only, so not a big deal imho.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true. And the copr package ends up in a lot of places. I don't think I'm smart enough to understand all the potential ramifications here. Let's see what others say.

vrothberg
vrothberg reviewed Sep 25, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/hold
@lsm5 merge at will

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 25, 2023
@lsm5
Copy link
Member Author

lsm5 commented Sep 25, 2023

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2023
@openshift-merge-robot openshift-merge-robot merged commit ca1cd02 into containers:main Sep 25, 2023
43 checks passed
@lsm5 lsm5 deleted the copr-rpm-version-sha branch September 25, 2023 12:56
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Dec 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@vrothberg vrothberg vrothberg left review comments

@cevich cevich cevich left review comments

Assignees

@vrothberg vrothberg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lsm5 @martinpitt @benoitf @cevich @vrothberg @TomSweeneyRedHat @openshift-merge-robot