-
Notifications
You must be signed in to change notification settings - Fork 104
feat: add registries conf #1008
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1008 +/- ##
==========================================
- Coverage 82.54% 82.44% -0.11%
==========================================
Files 64 65 +1
Lines 4779 4934 +155
==========================================
+ Hits 3945 4068 +123
- Misses 510 529 +19
- Partials 324 337 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements support for the registries.conf file format, extending the credentials package to handle both Docker config.json and containers registries.conf configurations. It introduces a Config interface to abstract different configuration formats and reorganizes the code structure for better modularity.
Key changes:
- Introduces
registriesConftype that implements TOML-based configuration handling - Adds a
Configinterface to abstract different configuration formats - Reorganizes the credentials package structure by moving types and splitting functionality
- Adds the NewStoreFromRegistriesConf function for creating stores from registries.conf files
Reviewed Changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| registry/remote/credentials/registries_conf.go | Implements TOML-based registries.conf parsing and management |
| registry/remote/credentials/config.go | Defines the Config interface for abstracting configuration formats |
| registry/remote/credentials/auth_config.go | Extracted auth configuration handling from config_json.go |
| registry/remote/credentials/credential.go | Moved credential types and functions from auth package |
| registry/remote/credentials/store.go | Updated to use Config interface and added registries.conf support |
| registry/remote/credentials/config_json.go | Refactored to implement Config interface |
| Multiple test files | Updated imports and type references after package reorganization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
54dc638 to
43d3a29
Compare
|
Looks like this depends on #1006 |
Yes. I'm going to convert this to a draft for now. |
43d3a29 to
d118dd9
Compare
f25284d to
434bb45
Compare
|
Rebased the commit on main instead of the credential change. |
434bb45 to
af65a10
Compare
|
Just moved the directory to internal and rebased |
sabre1041
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Question regarding the TOML library implementation
go.mod
Outdated
| require ( | ||
| github.com/opencontainers/go-digest v1.0.0 | ||
| github.com/opencontainers/image-spec v1.1.1 | ||
| github.com/pelletier/go-toml/v2 v2.2.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts about swapping the TOML parsing libraries to github.com/BurntSushi/toml? This is the library that is used by the container tools as well as Helm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Signed-off-by: Terry Howe <[email protected]>
af65a10 to
95e31f4
Compare
sabre1041
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
shizhMSFT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is containers-registries.conf cross-platform? If not, you need to document the OS limitation.
| go 1.24.0 | ||
|
|
||
| require ( | ||
| github.com/BurntSushi/toml v1.4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest version is v1.6.0.
| limitations under the License. | ||
| */ | ||
|
|
||
| package configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to be config instead of configuration.
| ) | ||
|
|
||
| // RegistriesConfig represents a registries.conf configuration file. | ||
| // Reference: https://github.com/containers/image/blob/main/docs/containers-registries.conf.5.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use link from tags or commits and never with main branch as the link may easily break.
| limitations under the License. | ||
| */ | ||
|
|
||
| package configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of having this package in internal? Should it be registry/remote/config?
Add support for registries.conf. This is not a full implementation it'll be a follow-up PR with more features for the file. This does contain basic support though.
Based on https://github.com/containers/image/blob/main/docs/containers-registries.conf.5.md
More detail https://man.archlinux.org/man/containers-registries.conf.5.en
This PR depends on #1006
Closes: #918
Closes: #1058