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

Moving policy.json and default.yaml from containers/skopeo #1757

Conversation

rahilarious
Copy link

All major distros (Fedora, debian, archlinux, gentoo, alpine, opensuse) are placing these 2 files in containers-common package. Why not fix it upstream?

2nd step towards addressing containers/skopeo#2170

All major distros (Fedora, debian, archlinux, gentoo, alpine,
opensuse) are placing these 2 files in containers-common package. Why
not fix it upstream?

2nd step towards addressing containers/skopeo#2170

Signed-off-by: Rahil Bhimjiani <[email protected]>
Copy link
Contributor

openshift-ci bot commented Dec 1, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rahilarious
Once this PR has been reviewed and has the lgtm label, please assign baude for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@rhatdan
Copy link
Member

rhatdan commented Dec 2, 2023

@mtrmac @vrothberg PTAL

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

I have no opinion on the location of these files, I’ll fully defer to our packaging experts (and this must be coordinated with them).

@lsm5 @jnovy PTAL.

@lsm5
Copy link
Member

lsm5 commented Dec 4, 2023

So far, there hasn't been much of a connection between this upstream repo and the containers-common package. The distro package only fetches the config files and manpages from a bunch of repos, currently common, image, shortnames, storage, and skopeo.

IIUC, we won't be able to move the conf files from the other libraries to this repo, would we? If that's the case, I weak-support this change in that it reduces the number of repos to pull files from, but brings on additional packaging changes which packagers of distros apart from fedora/rhel also need to be informed about.

@rhatdan
Copy link
Member

rhatdan commented Dec 4, 2023

I prefer to keep the man pages and config files with the libraries that use them. That way they can be tested their and containers-common can just collect them, and sometimes override them with things like RHEL support.

@rahilarious rahilarious changed the title Receiving policy.json and default.yaml from containers/skopeo Moving policy.json and default.yaml from containers/skopeo Dec 4, 2023
@rahilarious
Copy link
Author

rahilarious commented Dec 4, 2023

I prefer to keep the man pages and config files with the libraries that use them. That way they can be tested their and containers-common can just collect them, and sometimes override them with things like RHEL support.

I get this. This approach makes sense for libraries. That's why I'm all for pulling config from c/storage, c/image, c/shortnames but in case of skopeo, it is not only library but user-facing program too.

Podman, buildah also needs policy.json and default.yaml so distros have to install it in common. But the problem happens when make install in skopeo also install these 2 files. So it creates conflicts between common & skopeo. So packager needs to manually patch Makefile.

Example https://github.com/gentoo/gentoo/blob/e0512a7cc43063f2bd1348f965332adf66e98430/app-containers/skopeo/files/makefile-1.14.0.patch

and its bash https://github.com/gentoo/gentoo/blob/e0512a7cc43063f2bd1348f965332adf66e98430/app-containers/skopeo/skopeo-1.14.0-r1.ebuild

@rahilarious
Copy link
Author

brings on additional packaging changes which packagers of distros apart from fedora/rhel also need to be informed about.

The job of maintainer/packager is to check the release notes and adapt. Mentioning this change upon relaese should be enough. That shouldn't cause prevention of changes. :-)

@rhatdan
Copy link
Member

rhatdan commented Dec 4, 2023

Ok since these are just from Skopeo then we can move them out into containers/common.

@Luap99
Copy link
Member

Luap99 commented Dec 5, 2023

But aren't these file formats defined in c/image? So storing them in c/common doesn't make sense to me either. The man pages live in c/image so I see no reason why the default files shouldn't be there as well.

@rhatdan
Copy link
Member

rhatdan commented Dec 5, 2023

Sure move them to containers/image then.

rahilarious pushed a commit to rahilarious/image that referenced this pull request Dec 7, 2023
It makes more sense to keep these 2 files along with their man
pages...in c/image
containers/common#1757

Signed-off-by: Rahil Bhimjiani <[email protected]>
@rahilarious rahilarious closed this Dec 7, 2023
rahilarious pushed a commit to rahilarious/image that referenced this pull request Dec 9, 2023
It makes more sense to keep these 2 files along with their man
pages...in c/image
containers/common#1757

Signed-off-by: Rahil Bhimjiani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants