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.

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

[v4.5 backport][CI:BUILD] Packit: add jobs for downstream Fedora package builds #18675

Merged
merged 3 commits into from
May 25, 2023

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented May 24, 2023

Get rid of podman.spec.rpkg in favour of
rpm/podman.spec which gets synced with fedora dist-git on every upstream release. The version in the new spec file is set to 0 by default and gets updated by packit automatically on every packit task.

For local manual rpm builds using the spec, the helper script in the rpm/ subdir will update the Version field with the latest version found in the upstream repo.

Packit will automatically create a PR on fedora dist-git on every new upstream release. A sample PR will look like:
https://src.fedoraproject.org/rpms/container-selinux/pull-request/10#

A dry run for this can be triggered using:
$ packit propose-downstream --local-content

To run this command locally, you would need to have your packit user-configuration-file set.
Ref: https://packit.dev/docs/configuration/#user-configuration-file

along with a fedora api key created at:
https://src.fedoraproject.org/settings#nav-api-tab with sufficient ACLs.

Also includes a revised package Makefile target which will build rpms using rpm/podman.spec. Fixes: #18421.

[NO NEW TESTS NEEDED]

(cherry picked from commit 6003dca)

Does this PR introduce a user-facing change?

None

Get rid of `podman.spec.rpkg` in favour of
`rpm/podman.spec` which gets synced with fedora dist-git on every
upstream release. The version in the new spec file is set to `0` by
default and gets updated by packit automatically on every packit task.

For local manual rpm builds using the spec, the helper script in the
`rpm/` subdir will update the Version field with the latest version
found in the upstream repo.

Packit will automatically create a PR on fedora dist-git on every new
upstream release. A sample PR will look like:
https://src.fedoraproject.org/rpms/container-selinux/pull-request/10#

A dry run for this can be triggered using:
`$ packit propose-downstream --local-content`

To run this command locally, you would need to have your packit
user-configuration-file set.
Ref: https://packit.dev/docs/configuration/#user-configuration-file

along with a fedora api key created at:
https://src.fedoraproject.org/settings#nav-api-tab with sufficient ACLs.

Also includes a revised `package` Makefile target which will build rpms
using `rpm/podman.spec`. Fixes: containers#18421.

[NO NEW TESTS NEEDED]

Signed-off-by: Lokesh Mandvekar <[email protected]>
(cherry picked from commit 6003dca)
Signed-off-by: Lokesh Mandvekar <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 24, 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 May 24, 2023
@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

@cevich you cool with disabling cirrus task for podman-next on maint branches ? (RE: second commit on this PR)

@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

actually, i don't see any reason to have cirrus task for podman-next on any branch. Correct me if i'm wrong.

@cevich
Copy link
Member

cevich commented May 24, 2023

you cool with disabling cirrus task for podman-next on maint branches

Yep...though I question why other stuff is being updated on a maintenance branch when (my understanding is) this only applies to the latest/greatest main?

actually, i don't see any reason to have cirrus task for podman-next on any branch. Correct me if i'm wrong.

You would know better than I. My (limited) understanding is the build (test and otherwise) are all handled by the Packit service, including for PRs, yes? If so, then, yep, feel free to nuke it everywhere.

@cevich
Copy link
Member

cevich commented May 24, 2023

Also, I see Packit runs on this PR for F37/F38 but this release branch's CI is setup for F36/37. That seems like a recipe for future headaches. Can packit be blocked from running on other branches besides main? or is there a reason for it to run?

@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

Yep...though I question why other stuff is being updated on a maintenance branch when (my understanding is) this only applies to the latest/greatest main?

This is for the downstream Fedora tasks that will get triggered after an upstream release is published. The tasks need to be on the branch from which the release is cut. If you look at the trigger: commit task that was present already here, that is for a separate copr rhcontainerbot/qm and not our regular copr rhcontainerbot/pomdan-next. The qm copr is for people who wanna do testing with https://github.com/containers/qm and a relatively new and stable podman on CentOS 9 environments.

You would know better than I. My (limited) understanding is the build (test and otherwise) are all handled by the Packit service, including for PRs, yes? If so, then, yep, feel free to nuke it everywhere.

Yes, none of the rpm build tasks need to be done via cirrus. Thanks!

Also, I see Packit runs on this PR for F37/F38 but this release branch's CI is setup for F36/37. That seems like a recipe for future headaches. Can packit be blocked from running on other branches besides main? or is there a reason for it to run?

Packit CI is independent of Cirrus and directly uses the actively supported official Fedora branches. So what we specify in Cirrus doesn't matter to packit. The reason I asked about the cirrus task for podman-next was that it was failing on an earlier run on this PR, hence the additional commit to disable it.

@lsm5 lsm5 requested a review from edsantiago May 24, 2023 17:34
@cevich
Copy link
Member

cevich commented May 24, 2023

Packit CI is independent of Cirrus and directly uses the actively supported official Fedora branches.

Yep I get that, what I was wondering about is the potential for drift. Imagine a CVE comes out and needs a patch made on this branch a year from now. Cirrus-CI will validate the fix against F36/37 then packet will try and build for F38/39. That build could fail if there's any incompatibility with any dependency.

Is this concern real? Maybe is it possible to lock packit for this branch to only building F36/37 & Stream-8/9?

@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

Packit CI is independent of Cirrus and directly uses the actively supported official Fedora branches.

Yep I get that, what I was wondering about is the potential for drift. Imagine a CVE comes out and needs a patch made on this branch a year from now. Cirrus-CI will validate the fix against F36/37 then packet will try and build for F38/39. That build could fail if there's any incompatibility with any dependency.

Is this concern real? Maybe is it possible to lock packit for this branch to only building F36/37 & Stream-8/9?

Hmm, looks like I mixed up terminology earlier. This branch would be a release branch and not a maint branch. The -rhel branches would be maint branch. The packit config so far is only intended for Copr and Fedora tasks. If we need build checks on -rhel branches, I can update the packit config on those branches to only run build checks on centos stream 8 and 9 envs. We only need pre-commit CI build checks done via packit on the maint branches.

So, the CVE concern would be real for -rhel, but not for the release branch. Does that answer your q?

@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

Another thing to note about Copr infrastructure, copr follows Fedora's EOL policy. So, once fedora 36 goes EOL, the build env for f36 will vanish from copr. But anyway, any fedora release shouldn't be a concern for more than a year, CVE wise. Once v4.6 branch is created, we can stop caring about v4.5 branch for Fedora.

@cevich
Copy link
Member

cevich commented May 24, 2023

Yeah, it's the -rhel case I was thinking about. I like your idea of limiting those to Cent, that would decouple it from the fast-moving Fedora.

Conceptually this SGTM (I haven't looked at any of the actual file changes).

@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

Yeah, it's the -rhel case I was thinking about. I like your idea of limiting those to Cent, that would decouple it from the fast-moving Fedora.

Great, thanks. Let me know if I should go ahead on any existing -rhel branches.

Conceptually this SGTM (I haven't looked at any of the actual file changes).

No worries. I subjected @edsantiago to that pain on the main branch already :D

Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

A few minor bash-concerns for consideration.

rpm/Makefile Show resolved Hide resolved
rpm/Makefile Outdated Show resolved Hide resolved
rpm/update-spec-provides.sh Show resolved Hide resolved
rpm/update-spec-version.sh Show resolved Hide resolved
@edsantiago
Copy link
Member

LGTM pending @cevich's concerns

Also address review concerns in pr#18675.

[NO NEW TESTS NEEDED]

Co-authored-by: Chris Evich <[email protected]>
Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 force-pushed the v4.5-packit-downstream branch from a700d93 to 2b045de Compare May 25, 2023 13:26
@lsm5
Copy link
Member Author

lsm5 commented May 25, 2023

@edsantiago @cevich addressed all concerns in a new commit here and I also have a PR with those changes at #18687 for the main branch . I uncommented a few lines in .packit.yaml after checking with the packit people, so there's some additional diff.

@edsantiago
Copy link
Member

/lgtm
/hold

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

mheon commented May 25, 2023

LGTM
/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 May 25, 2023
@openshift-merge-robot openshift-merge-robot merged commit 63d06bc into containers:v4.5 May 25, 2023
@lsm5 lsm5 deleted the v4.5-packit-downstream branch May 25, 2023 17:33
@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 Aug 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

5 participants