[skip-ci] Packit: New workflow for downstream Fedora and CentOS Stream 10 #1960
Conversation
|
Ephemeral COPR build failed. @containers/packit-build please check. |
lsm5
changed the title
lsm5
force-pushed
the
rpm-packit
branch
7 times, most recently
from
6cf8857
to
91b8a31
Compare
|
Failed to load packit config file: For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
2 similar comments
|
Failed to load packit config file: For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
|
Failed to load packit config file: For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
lsm5
force-pushed
the
rpm-packit
branch
5 times, most recently
from
734eee7
to
59b69e6
Compare
|
@rhatdan @Luap99 @cevich @edsantiago @jnovy @mheon @mtrmac @TomSweeneyRedHat @nalind PTAL podman and toolbox work fine for me with the rpms generated in the rawhide copr build job. Would be great if you could give these a try too. You can try this out with: |
|
cc @inknos if you'd like to try this out too |
| %global image_branch main | ||
| %global storage_branch main | ||
| %global shortnames_branch main |
There was a problem hiding this comment.
Who is going to do that? I think we have the issue today that many config files do not actually match the tagged versions 1 to 1.
go.mod should already specify the correct version so can we use this to automate this somehow?
There was a problem hiding this comment.
added a packit action that will run rpm/update-lib-versions.sh on downstream PRs. We can only test this once downstream PRs are created though.
There was a problem hiding this comment.
Thanks this looks great assuming it works as expected
rpm/containers-common.spec
Outdated
| # Patch storage.conf | ||
| sed -i -e 's/^driver.*=.*/driver = "overlay"/' -e 's/^mountopt.*=.*/mountopt = "nodev,metacopy=on"/' \ | ||
| -e '/additionalimage.*/a "/usr/lib/containers/storage",' \ | ||
| %if %{defined copr_build} || (%{defined fedora} && 0%{?fedora} > 40) | ||
| -e 's/^pull_options.*=.*/pull_options = {enable_partial_images = \"true\", use_hard_links = \"false\", ostree_repos="", convert_images = "false"}/' \ | ||
| -e 's/# use_composefs.*/use_composefs = "false"/g' \ | ||
| %endif | ||
| storage.conf | ||
|
|
||
| # Patch seccomp.json | ||
| [ `grep "keyctl" pkg/seccomp/seccomp.json | wc -l` == 0 ] && sed -i '/\"kill\",/i \ | ||
| "keyctl",' pkg/seccomp/seccomp.json | ||
| sed -i '/\"socketcall\",/i \ | ||
| "socket",' pkg/seccomp/seccomp.json | ||
|
|
||
| # Patch registries.conf | ||
| sed -i 's/^#.*unqualified-search-registries.*=.*/unqualified-search-registries = ["registry.fedoraproject.org", "registry.access.redhat.com", "docker.io"]/g' \ | ||
| registries.conf | ||
| grep '^short-name-mode="enforcing"' registries.conf && echo -e '\nshort-name-mode="enforcing"' >> registries.conf | ||
|
|
||
| # Patch containers.conf | ||
| sed -i -e 's/^#.*log_driver.*=.*/log_driver = "journald"/' \ | ||
| -e 's/^#.*compression_format.*=.*/compression_format = "zstd:chunked"/' \ | ||
| pkg/config/containers.conf |
There was a problem hiding this comment.
I think we should review this? Why do we patch our own upstream config files so heavily.
I understand that some of them make sense, i.e. adding fedora registries but other are not so obvious. I know this is juts copying the function out of the rpm of today but I really think these should have a clear explanation WHY they are patched in this way.
There was a problem hiding this comment.
@rhatdan I lifted these from fedora dist-git. How many of these patches are safe to merge/enable upstream?
There was a problem hiding this comment.
my only 2 cents on this PR would be that scripts could be handled out of the specfile. that's mostly a personal preference as I don't think there's any recommendation on this.
There was a problem hiding this comment.
@inknos ack thanks. We've been doing something similar in dist-git so far https://src.fedoraproject.org/rpms/containers-common/blob/rawhide/f/update.sh . Let's check if we can reduce these further.
There was a problem hiding this comment.
(I know, this is not the topic of the PR, feel free to ignore.)
RPM has a native %patch (%autopatch?) mechanism. It seems to me that doing this with manual sed is all much less readable, and more risky to do something unexpected without us noticing, and I don’t see any benefit.
I tend to think that patch conflicts and application failures are actually our friends.
Does anyone here immediately know what -e '/additionalimage.*/a "/usr/lib/containers/storage",' does without referring to the storage.conf being edited?!
I also agree that if Fedora downstream is using non-default options, that is quite possible but perhaps non-obvious. So having some documentation about which changes are
- forward-ports where we force rawhide to be more experimental than upstream releases
- backports where upstream is ahead of a stable version in a RPM
- long-term differences
along with reasons, would help future maintainers. Or, well, current maintainers asking “How many of these patches are safe to merge/enable upstream?”.
Especially if we move the location where this is maintained, thus requiring extra knowledge to find the original git history.
FWIW, non-authoritative, just to start the discussion:
-e '/additionalimage.*/a "/usr/lib/containers/storage",' \feels like CoreOS/image-based-OS specific, not a good upstream default (but I’m not 100% sure)enable_partial_imagesetc.,"zstd:chunked"are experimental and not upstream-ready at this point (ref . zstd:chunked blocker: TarSplitChecksumKey not used in a layer ID storage#1888 )unqualified-search-registriesare Fedora-specific to keep compatibility with older Fedora users, less secure than upstream default- I don’t immediately know anything about the others (and I didn’t go looking)
There was a problem hiding this comment.
Does anyone here immediately know what
-e '/additionalimage.*/a "/usr/lib/containers/storage",'does without referring to thestorage.confbeing edited?!
there's storage.conf on line 115. Sorry if the readability is poor currently.
And ack other suggestions too.
There was a problem hiding this comment.
RPM has a native %patch (%autopatch?) mechanism. It seems to me that doing this with manual sed is all much less readable, and more risky to do something unexpected without us noticing, and I don’t see any benefit.
Sorry for being late to the discussion. +1 for migrating to spec file patches as much as possible in the future.
lsm5
force-pushed
the
rpm-packit
branch
2 times, most recently
from
1d883f4
to
8c56f4f
Compare
lsm5
changed the title
This commit enables: - upstream copr build jobs on PRs - rpm builds on podman-next copr after every commit to main - Fedora and CentOS Stream 10 downstream update jobs on every upstream release - Fetch RPM-GPG-KEY-redhat-release from https://access.redhat.com/security/data/fd431d51.txt - Config file patching during rpm build is managed via `rpm/update.sh`, modified from the original script at https://gitlab.com/redhat/centos-stream/rpms/containers-common/-/blob/c9s/update.sh and modified to include Fedora and RHEL-10 Co-authored-by: Ed Santiago <[email protected]> Signed-off-by: Lokesh Mandvekar <[email protected]>
|
@edsantiago PTAL, comments addressed. @mheon @TomSweeneyRedHat before we cut a new c/common, I'm hoping to have this change from @ktdreyer reviewed, should come in a new PR once this PR is merged. |
|
/lgtm Thank you for your patience! |
|
@edsantiago thanks, looks like it needs a separate slash-approve too |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, lsm5, Luap99 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 |
The copr name got mistakenly changed during a replace-all in containers#1960. Signed-off-by: Lokesh Mandvekar <[email protected]>
The copr name got mistakenly changed during a replace-all in containers#1960. Signed-off-by: Lokesh Mandvekar <[email protected]>
|
/cherrypick v0.58 |
The copr name got mistakenly changed during a replace-all in containers#1960. Signed-off-by: Lokesh Mandvekar <[email protected]> (cherry picked from commit 1a2d001) Signed-off-by: Lokesh Mandvekar <[email protected]>
This commit enables:
release
rpm/update-config-files.sh,modified from the original script at
https://gitlab.com/redhat/centos-stream/rpms/containers-common/-/blob/c9s/update.sh
and modified to include Fedora and RHEL-10