add internal fork of docker/docker/registry #6207
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
This adds an internal fork of [github.com/docker/docker/registry], taken at commit [moby@f651a5d]. Git history was not preserved in this fork, but can be found using the URLs provided. This fork was created to remove the dependency on the "Moby" codebase, and because the CLI only needs a subset of its features. The original package was written specifically for use in the daemon code, and includes functionality that cannot be used in the CLI. [github.com/docker/docker/registry]: https://pkg.go.dev/github.com/docker/docker@v28.3.2+incompatible/registry [moby@49306c6]: https://github.com/moby/moby/tree/49306c607b72c5bf0a8e426f5a9760fa5ef96ea0/registry Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The CLI does not have information about mirrors, and doesn't configure them, so we can remove these parts. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's not matched anywhere, so we can just return a plain error. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was only used in a single place; inline it there. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
now that we no longer need to account for mirrors, these were identical, so just use a single one. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was written to be used as validate-func for command-line flags, which we don't use it for (which for CLI-flags includes normalizing the value). The validation itself didn't add much; it only checked the registry didn't start or end with a hyphen (which would still fail when parsing). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The registry.ServiceConfig struct in the API types was meant for the registry configuration on the daemon side; it has variuos fields we don't use, defines methods for (un)marshaling JSON, and a custom version of `net.IPNet`, also to (un)marshal JSON. None of that is needed, so let's change it to a local type, and implement a constructor (as we now only have "insecure registries" to care about). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
internal/registry/errors.go:26:43: use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)
func invalidParamf(format string, args ...interface{}) error {
^
internal/registry/registry_mock_test.go:52:51: use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)
func writeResponse(w http.ResponseWriter, message interface{}, code int) {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There was a problem hiding this comment.
We should look at refactoring this test code, it uses init() which isn't necessary and we can move the using stretchr/testify/assert instead of t.Fatalf.
There was a problem hiding this comment.
Yes, I didn't look closely at the tests yet. Initially I forked without tests, but then decided to "let's just copy those as well - just in case it's covering some bits that are important"
There was a problem hiding this comment.
we import assert in this file but we end up using t.Fatalf instead (perhaps theses should be require instead?)
There was a problem hiding this comment.
Yup! See above; depending on what we end up doing with this package (probably should be fully dismantled), we should either fully use assert, or fully stdlib testing - depending on what's needed.
or library packages that only need minimal functionality of an assert library, I try to use stdlib ("zero dependencies!"). But if things get too complicated with that, using an assert library is OK.
| log.G(ctx).WithFields(log.Fields{ | ||
| "error": err, | ||
| "endpoint": endpoint, | ||
| }).Infof("Error logging in to endpoint, trying next endpoint") |
There was a problem hiding this comment.
does this logger actually do something in the CLI?
There was a problem hiding this comment.
It does; it (currently) falls back to using the default logger if none was set on the context (which I actually want to change in that package - or at least an easier way to set a noplogger / discardlogger);
cli/vendor/github.com/containerd/log/context.go
Lines 175 to 182 in 636a4cf
It's a bit awkward, but log.G is a variable, so allows anyone to swap it for anything. I thought that was a design-mistake, so at some point changed it, but turned out that BuildKit found that loophole and is abusing it;
cli/vendor/github.com/containerd/log/context.go
Lines 47 to 51 in 636a4cf
|
I'll bring this one in, but more cleaning up may follow; also to get rid of some of the types that are used, and (currently) must remain related to Docker Content Trust code expecting it, but I'd love to get rid of those where possible, and simplify for it to be "just enough for what we need".
|
|
☝️ for context on the above;
But the implementation in the registry was tightly coupled to the registry API server, so now we ended up having (e.g.) |
add internal fork of docker/docker/registry
This adds an internal fork of github.com/docker/docker/registry, taken
at commit moby@f651a5d. Git history was not preserved in this fork,
but can be found using the URLs provided.
This fork was created to remove the dependency on the "Moby" codebase,
and because the CLI only needs a subset of its features. The original
package was written specifically for use in the daemon code, and includes
functionality that cannot be used in the CLI.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)