image/docker: use unified configfile for registries.d#753
image/docker: use unified configfile for registries.d#753mtrmac merged 2 commits intocontainers:mainfrom
Conversation
github-actions
Bot
added
storage
|
This depends on #711. If someone wants to review this early, check only the last commit. |
|
Just a note to myself and others: this PR is waiting for new Read option from #773. |
Luap99
added
the
podman 6
jankaluza
force-pushed
the
registries.d-2
branch
from
470e8ba to
16bb53e
Compare
|
The skopeo tests are broken with this PR. I'm investigating why. |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Not a review yet, just things a LLM found
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
A full review now, ACK overall.
The skopeo tests are broken with this PR. I'm investigating why.
Yes, those failures look worrying… they don’t suggest that the configuration loading code is wrong, but at a glance I can’t see how a ~legitimately missing directory should cause a failure like that (but I didn’t investigate at all).
| func writeDockerLookaside(t *testing.T, dir, filename, registry, lookaside string) { | ||
| t.Helper() | ||
| require.NoError(t, os.WriteFile(filepath.Join(dir, filename), []byte(fmt.Sprintf("docker:\n %s:\n lookaside: %s\n", registry, lookaside)), 0o644)) | ||
| } | ||
|
|
||
| func writeDefaultDockerLookaside(t *testing.T, dir, filename, lookaside string) { | ||
| t.Helper() | ||
| require.NoError(t, os.WriteFile(filepath.Join(dir, filename), []byte(fmt.Sprintf("default-docker:\n lookaside: %s\n", lookaside)), 0o644)) | ||
| } |
There was a problem hiding this comment.
(Non-blocking: These are ~local to one test, so they could be placed near that test or even nested in it.)
jankaluza
force-pushed
the
registries.d-2
branch
from
16bb53e to
ecb12cf
Compare
|
@Luap99 , @mtrmac , OK, the skopeo test is failing, because it creates the registries.yaml as a symlink, but we only support "regular" files in configfile: https://github.com/containers/container-libs/blob/main/storage/pkg/configfile/parse.go#L387 I will extend that to also allow symlinks if that's fine by you. |
|
That fixed skopeo tests. The remaining test failures are infra issues :-(. |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
LGTM, apart from Paul’s review.
Replace the custom registries.d loader with `configfile.File` and `configfile.Read`, aligning registry configuration with the standard drop-in handling used elsewhere. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
The symlinks and other non-regular files were supported by the previous code loading registries configuration files. This commit brings this support back by changing the directory entry check from `IsRegular()` to `!IsDir()`. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
jankaluza
force-pushed
the
registries.d-2
branch
from
b9f7cec to
0b40258
Compare
|
The usual question did you run this through podman/buildah CI? Just to ensure we do not need any code or test fixes. |
|
@Luap99 , thanks for reminder:
It's still running. |
|
I will merge once containers/podman#28570 lands unless @mtrmac finds something here in the meantime. |
3 participants
Switch registries.d loading to use
configfile.Read(), enablingunified drop-in search across /usr, /etc, and user config directories.
Files are merged with standard precedence, with higher-priority paths
masking lower ones.
Preserve explicit RegistriesDirPath override behavior.
Signed-off-by: Jan Kaluza jkaluza@redhat.com