Packit: initial enablement #4
Conversation
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
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 |
Signed-off-by: Lokesh Mandvekar <[email protected]>
|
@jnovy @jankaluza @Luap99 @mtrmac PTAL |
|
What is the play here exactly regarding PRs, if I understand @jankaluza plan correctly the entire repo will be wiped and recreated on the final day so all changes here are lost? Or do we create a patchset of all the changes and then reapply that to the new tree again? |
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
LGTM as far as the changes itself go though I haven't carefully reviewed the spec changes.
| # not an option because that package doesn't exist on CentOS Stream. | ||
| Source15: https://access.redhat.com/security/data/fd431d51.txt | ||
| Source0: %{url}/archive/refs/tags/common/v%{version}.tar.gz | ||
| Source1: https://raw.githubusercontent.com/containers/shortnames/refs/heads/main/shortnames.conf |
There was a problem hiding this comment.
Shouldn't this be a part of container-libs too?
There was a problem hiding this comment.
that's a separate repo and looks like it's going to remain separate because there are contributors from other projects.
| # a copy in repo or dist-git. Depending on distribution-gpg-keys rpm is also | ||
| # not an option because that package doesn't exist on CentOS Stream. | ||
| Source15: https://access.redhat.com/security/data/fd431d51.txt | ||
| Source0: %{url}/archive/refs/tags/common/v%{version}.tar.gz |
There was a problem hiding this comment.
Why it's delivering the whole world and not only bits which are actually needed/installed for the container ecosystem? This is super wasteful in terms of package size, content and overall packaging.
There was a problem hiding this comment.
where exactly is the package size and content impact felt?
There was a problem hiding this comment.
`Source0: %{url}/archive/refs/tags/common/v%{version}.tar.gz'
There is a fundamental difference between a regular package which actually builds from source and this one. The build doesn't happen. The purpose of containers-common is to only deliver small configuration files originating from upstream to container-tools. For this purpose stashing not only config files but also sources - which are unbuilt - into the mothership tarball with 99.99% of stuff uninstalled is very wasteful.
There was a problem hiding this comment.
Sure, but who's exactly feeling the impact of this?
- Does it cause unacceptable increase in build time?
- Does the end user care?
- Does the distro care?
- Does the customer, for example CoreOS / OpenShift, care ?
The current PR makes the spec file a lot simpler. So, unless someone is actually feeling the impact of a large tarball, I'd prefer to keep it this way.
There was a problem hiding this comment.
"who cares?" doesn't seem the correct justification :-) Also it is not exactly a simplification for various reasons:
Consider this:
with tarball-only approach:
- one needs to patch config files on the fly in the spec file
- one needs to be super cautious to not to break container tools if you deploy containers-common which is not vendored into podman/buildah/skopeo yet
With per-version source URLs:
- one knows exactly what you package as the required config files are actually stored in dist-git with reference to upstream URL from which is originates from
- The update_vendored.sh script actually assures no regression possibility by design - it will set vendored versions of c/storage, c/image and c/common in the spec to pull configuration files from based on actual lowest common denominator of podman/buildah/skopeo vendored content
- The configuration files in 2) are comprehensibly updated with per-OS configuration by update.sh script so it is obvious to anyone what is different in e.g. RHEL8, RHEL9 and RHEL10 - the "ensure" bash functions
There was a problem hiding this comment.
"who cares?" doesn't seem the correct justification :-) Also it is not exactly a simplification for various reasons:
Consider this:
with tarball-only approach:
- one needs to patch config files on the fly in the spec file
which is what we've been already doing via the Packit workflow for Fedora for sometime now.
- one needs to be super cautious to not to break container tools if you deploy containers-common which is not vendored into podman/buildah/skopeo yet
That's why we have packit copr builds and TMT tests running on podman, buildah and skopeo PRs fetching containers-common from podman-next. So, things are continuously tested upstream.
With per-version source URLs:
- one knows exactly what you package as the required config files are actually stored in dist-git with reference to upstream URL from which is originates from
If any auditor cares, they're welcome to fetch and extract files from the rpm. No reason to add unnecessary maintenance burden downstream (Fedora). They're config files so they don't need to be stored in dist-git to be readable. CentOS Stream and RHEL process are your call.
- The update_vendored.sh script actually assures no regression possibility by design - it will set vendored versions of c/storage, c/image and c/common in the spec to pull configuration files from based on actual lowest common denominator of podman/buildah/skopeo vendored content
With monorepo, renovate and consistent tests running upstream and on Fedora, I don't see this being an issue. At the very least, nothing worth requiring any manual maintenance on Fedora.
- The configuration files in 2) are comprehensibly updated with per-OS configuration by update.sh script so it is obvious to anyone what is different in e.g. RHEL8, RHEL9 and RHEL10 - the "ensure" bash functions
That has been upstreamed already in common/rpm/update-config-files.sh. Again, CentOS Stream and RHEL are your call.
That's correct, but I will take all the changes we've done here and apply them on the migration day. I've also done some changes outside of my script already, because the repo changed slightly during last few months. |
|
We should hold off on this PR until vendor PRs are merged and renovate jobs are known to work. Setting this to draft until then. |
|
containers/podman#26858 seems to build copr rpms just fine and the renovate PRs on this repo are only bumping go.mod and go.sum. So, I'm removing draft status from this one. Please feel free to merge whenever convenient, unless any other pending concerns. |
|
No rush, but I think this needs to be reopened. |
|
You need to redo the PR/commit as the entire git repo was wiped. |
ah alright |
4 participants
No description provided.