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

Config files should live in /usr/local on FreeBSD (highlight in release notes!) #1544

Merged
merged 1 commit into from
May 10, 2022

Conversation

dfr
Copy link
Contributor

@dfr dfr commented May 8, 2022

Signed-off-by: Doug Rabson [email protected]

@dfr dfr force-pushed the freebsd branch 2 times, most recently from bd2987c to 75765ee Compare May 8, 2022 09:55
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

pkg/sysregistriesv2/system_registries_v2_darwin.go Outdated Show resolved Hide resolved
signature/policy_config_darwin.go Outdated Show resolved Hide resolved
Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM. Needs rebase though.

Also, do we need something FreeBSD specific in CI going forrward?

@dfr
Copy link
Contributor Author

dfr commented May 9, 2022

Good point - I can add something to Makefile as a followup PR.

Copy link
Collaborator

@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.

There are three other /etc paths in c/image/docker; should those be updated as well?


Compare containers/skopeo#1298 (comment) / containers/skopeo#1507 . Changing the default in c/image instead of Skopeo is definitely better, still…

  • Changing the default is going to immediately break Skopeo’s Makefile that installs config files to /etc.
  • How are upgrades going to work? Why isn’t this going to, potentially silently, ignore existing user’s configuration (with security implications like ignoring blocked registry configurations)? At least for users that used to compile themselves using e.g. Skopeo’s Makefile, this seems to me to be an unavoidable compatibility break. Or is there some reason why that isn’t actually possible, so it isn’t relevant?

@@ -0,0 +1,12 @@
//go:build !freebsd
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Absolutely non-blocking: Naming this pkg/sysregistriesv2/paths_{common,freebsd}.go would work just as well, and be a tiny bit more descriptive.)

@@ -0,0 +1,8 @@
//go:build !freebsd
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Absolutely non-blocking: Naming this pkg/sysregistriesv2/policy_paths_{common,freebsd}.go would work just as well, and be a tiny bit more descriptive.)

@dfr
Copy link
Contributor Author

dfr commented May 9, 2022

There are three other /etc paths in c/image/docker; should those be updated as well?

I don't think there is a fully functioning docker for FreeBSD at this point. I know some people were working on porting moby but I'm not sure of the status there.

Compare containers/skopeo#1298 (comment) / containers/skopeo#1507 . Changing the default in c/image instead of Skopeo is definitely better, still…

  • Changing the default is going to immediately break Skopeo’s Makefile that installs config files to /etc.
  • How are upgrades going to work? Why isn’t this going to, potentially silently, ignore existing user’s configuration (with security implications like ignoring blocked registry configurations)? At least for users that used to compile themselves using e.g. Skopeo’s Makefile, this seems to me to be an unavoidable compatibility break. Or is there some reason why that isn’t actually possible, so it isn’t relevant?

This is a good point but perhaps not a big issue - I think there are only a handful of people using skopeo on FreeBSD at this point since its not available as a pre-build package. I can follow up with a PR on skopeo to install to the correct path.

@mtrmac
Copy link
Collaborator

mtrmac commented May 9, 2022

There are three other /etc paths in c/image/docker; should those be updated as well?

I don't think there is a fully functioning docker for FreeBSD at this point

This is for the “container image registry” client (incl. podman pull), it has nothing to do with /usr/bin/docker.

@mtrmac
Copy link
Collaborator

mtrmac commented May 9, 2022

  • How are upgrades going to work? Why isn’t this going to, potentially silently, ignore existing user’s configuration (with security implications like ignoring blocked registry configurations)? At least for users that used to compile themselves using e.g. Skopeo’s Makefile, this seems to me to be an unavoidable compatibility break. Or is there some reason why that isn’t actually possible, so it isn’t relevant?

This is a good point but perhaps not a big issue - I think there are only a handful of people using skopeo on FreeBSD at this point since its not available as a pre-build package

I am unhappy about this in principle, but ultimately I do think it’s appropriate for FreeBSD contributors to decide on the FreeBSD tradeoffs…

RFC @mateuszkwiatkowski (also @vrothberg @lsm5 )

@mtrmac
Copy link
Collaborator

mtrmac commented May 9, 2022

Also, do we need something FreeBSD specific in CI going forrward?

At least to the extent Red Hat is paying for the CI, I wouldn’t say we need FreeBSD coverage. Still, it’s a good idea to explore this, probably in a separate PR. (I guess adding FreeBSD to the cross test would be a cheap and easy smoke test. Modifying all of Skopeo’s integration/system testing to run on FreeBSD seems intuitively hard, but I know very little about FreeBSD capabilities.)

@vrothberg
Copy link
Member

SGTM ✔️

@dfr
Copy link
Contributor Author

dfr commented May 10, 2022

I changed the file names along the lines suggested by @mtrmac and made a similar change in docker/.

For skopeo, I opened containers/skopeo#1642 which changes the Makefile to match. The Makefile in skopeo unconditionally overwrites policy.json and after the change will write the default policy to the new location. Are there release notes for skopeo releases? Perhaps the change can be mentioned there.

@rhatdan
Copy link
Member

rhatdan commented May 10, 2022

LGTM

@rhatdan rhatdan merged commit 0ef0695 into containers:main May 10, 2022
@dfr dfr deleted the freebsd branch May 10, 2022 15:14
@mtrmac
Copy link
Collaborator

mtrmac commented May 10, 2022

Thanks again!

@mtrmac mtrmac changed the title Config files should live in /usr/local on FreeBSD Config files should live in /usr/local on FreeBSD May 10, 2022
@mtrmac mtrmac changed the title Config files should live in /usr/local on FreeBSD Config files should live in /usr/local on FreeBSD (highlight in release notes!) May 10, 2022
@mtrmac
Copy link
Collaborator

mtrmac commented May 10, 2022

Are there release notes for skopeo releases? Perhaps the change can be mentioned there.

GitHub does have a release notes mechanism. We don’t currently have automated mechanism to collect/create them for c/image or Skopeo. I have, at least, re-titled the PRs; hopefully that helps us remember when the time comes.

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