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 default-policy.json and default.yaml to containers/image #2173

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rahilarious
Copy link
Contributor

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

1st step towards addressing #2170

@rahilarious rahilarious force-pushed the default-yaml-policy-json-to-common branch from 492ab56 to 441249f Compare December 1, 2023 23:04
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.


I do have one opinion: Skopeo’s tests must continue to work, and this causes a failure.

@@ -147,6 +147,7 @@ ostree-rs-ext_task:
- dnf builddep -y skopeo
- make
- make install
- echo '{"default":[{"type":"insecureAcceptAnything"}],"transports":{"docker-daemon":{"":[{"type":"insecureAcceptAnything"}]}}}' > /etc/containers/policy.json
Copy link
Contributor

Choose a reason for hiding this comment

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

  • I’m afraid this is failing
  • I’m tempted to say that this should do the same thing we are asking other users to do. I.e. if users are expected to install the files from the c/image repo, this should also go through the troubler of installing the files from the c/image repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* I’m tempted to say that this should do the same thing we are asking other users to do. I.e. if users are expected to install the files from the c/image repo, this should also go through the troubler of installing the files from the c/image repo.

I agree. I just don't know how to handle cirrus stuff. It's up to you to properly fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is… just a shell script?


… Uh. The one thing we might need to provide some guidance on is to figure out which c/image version to use. ~Luckily the data is available in go.mod, but turning that into a git command is a bit cumbersome.

@lsm5 @jnovy @rhatdan What do we do for Podman users who build from source, and need containers.conf from c/common? It seems to be either “nothing and let them figure it out” or “install a previous packaged of containers-common and hope the files match”, is there something I’m missing?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing but let them figure it out. Luckily Podman runs fine with no containers.conf and we encourage distributions to comment out containers.conf to be used information purposes only, IE Document the default in the system file, and allow admin and users to customize individual fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m afraid policy.json is mandatory. Oh well… I’m always happy to recommend using packaged versions :)

@mtrmac
Copy link
Contributor

mtrmac commented Jan 4, 2024

For the record, the files, and the Make rules to install them, were moved to containers/image#2215 .

@rahilarious rahilarious changed the title Migrating default-policy.json and default.yaml to containers/common Moving default-policy.json and default.yaml to containers/image Jan 5, 2024
@lsm5
Copy link
Member

lsm5 commented Sep 16, 2024

@mtrmac I guess we can revise this PR to handle any outstanding cleanup issues if @rahilarious is willing to continue this.

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

1st step towards addressing containers#2170

Signed-off-by: Rahil Bhimjiani <[email protected]>
@rahilarious rahilarious force-pushed the default-yaml-policy-json-to-common branch from 92df14c to 6b73165 Compare September 16, 2024 12:53
@rahilarious
Copy link
Contributor Author

@mtrmac I guess we can revise this PR to handle any outstanding cleanup issues if @rahilarious is willing to continue this.

I'm unfamiliar with cirrus stuff, any help would be appreciated

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lsm5
Copy link
Member

lsm5 commented Sep 16, 2024

@mtrmac I guess we can revise this PR to handle any outstanding cleanup issues if @rahilarious is willing to continue this.

I'm unfamiliar with cirrus stuff, any help would be appreciated

Ack, no worries. I'm working on enabling TMT tests in #1960 and if things work there we can probably get rid of cirrus tests that need changes here.

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.

4 participants