-
Notifications
You must be signed in to change notification settings - Fork 76
Add ability to configure registry-specific proxy in registries.conf
#650
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
mtrmac
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.
https://github.com/containers/container-libs/blob/main/CONTRIBUTING.md#sign-your-prs please, we can’t really even look at code with unclear copyright status.
Sorry about that. Was planning to do an interactive rebase when things are ready, so I didn't bother. Will fix tomorrow. |
27a5643 to
f82f918
Compare
|
Packit jobs failed. @containers/packit-build please check. |
1 similar comment
|
Packit jobs failed. @containers/packit-build please check. |
Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
f82f918 to
bc60b07
Compare
|
Still missing a test in |
mtrmac
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.
Thanks!
I agree this makes sense to do for Podman accessed through the remote API, and the config file addition looks good.
|
|
||
| registries, err := GetRegistries(sys) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 2, len(registries)) |
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.
(Non-blocking: I suspect right now all of this could be a single assert.Equal against a literal, instead of 10 individual checks. But add the *net.URL field first before trying that transformation.)
|
Thanks a lot for the very detailed review! This is actually my first proper exercise in production Go so I really appreciate the constructive criticism. I'm more of a Rust guy, and coming into Go I have to say the verbose error handling and the lack of ADT (and hence the presence of invalid states) really irks me. On the other hand, the freedom to just pass around "dangling" references without having to worry about lifetimes is a nice convenience. Anyways I digress. Please give me a moment to go through all the recommendations. |
Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
- Rewrite comments for `registryProxy` to make it more appropriate for its layer - Make comments regarding loading registry config more substantive Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
1746c98 to
963a509
Compare
Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
Because it has a narrower scope than the globally scoped env vars. Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
2b4c1b3 to
5f58f16
Compare
I've been attempting to write this test for a couple of hours now and I've found it to be particularly difficult to set up. At this point, given the minimal amount of additional logic in What do you think? |
I think that’s fine. Generally the code in (Also, it would be easy enough to split the |
|
|
||
| Each TOML table in the `mirror` array can contain the following fields: | ||
| - `location`: same semantics | ||
| as specified in the `[[registry]]` TOML table | ||
| - `insecure`: same semantics | ||
| as specified in the `[[registry]]` TOML table | ||
| - `proxy`: same semantics | ||
| as specified in the `[[registry]]` TOML table | ||
| - `pull-from-mirror`: `all`, `digest-only` or `tag-only`. If "digest-only", mirrors will only be used for digest pulls. Pulling images by tag can potentially yield different images, depending on which endpoint we pull from. Restricting mirrors to pulls by digest avoids that issue. If "tag-only", mirrors will only be used for tag pulls. For a more up-to-date and expensive mirror that it is less likely to be out of sync if tags move, it should not be unnecessarily used for digest references. Default is "all" (or left empty), mirrors will be used for both digest pulls and tag pulls unless the mirror-by-digest-only is set for the primary registry. | ||
| Note that this per-mirror setting is allowed only when `mirror-by-digest-only` is not configured for the primary registry. | ||
|
|
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.
I don't think this list gets formatted correctly but that change would not belong here.
Signed-off-by: cyqsimon <28627918+cyqsimon@users.noreply.github.com>
96e3dcc to
af0e3ea
Compare
|
Okay I think all is good now. Please see if you're happy with this current state, and let me know if you would like me to do any sort of rebase and/or squash. |
Closes #624.
Checklist