-
Notifications
You must be signed in to change notification settings - Fork 80
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
OCI fallback #159
OCI fallback #159
Conversation
type Client struct{} | ||
|
||
func New() (*Client, error) { | ||
return &Client{}, nil | ||
} |
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 think we could construct a *remote.Puller
here and use it for the remote operations in Tags
. This will cache auth across different calls, reducing the potential number of requests made for each operation.
See:
https://github.com/google/go-containerregistry/blob/main/pkg/v1/remote/puller.go
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.
Yeah, I had this in mind for the future in building this out to be a fully fledged client in it's own right, but I didn't want to do it here as a first pass - mostly I try to work in incremental value, rather than hitting too many things at once.
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.
Left some comments.
Two other things worth mentioning:
- Auth. By default, ggcr will try to find credentials in the environment, for instance in
~/.docker/config.json
. I reckon we should document this, so users are aware that they can make creds available like that. - Tests. It's pretty straightforward and powerful to use the the ggcr registry for testing. You can see an example of that in paranoia.
OCI registries don't provide a way to retrieve metadata in the same call as listing tags. This means that you have to do a separate API request for each tag if you want the metadata, which could be very slow for a large number of tags. As such, preferably use the selfhosted client as a fallback, but add a fallback fallback to the oci handler for registries that are incompatible with the selfhosted API implementation.
As we might not have retrieved a SHA (dependent on registry type), don't try to include one if it's empty.
I'll probably look to add tests in a separate PR - As there's not currently a comprehensive testing framework, I think bringing tests into this PR might make it difficult to review. |
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.
Now that I've had chance to review.. Looks good to me - Appreciate the approach of falling back and keeping others - I may have some time to complete the auth and caching... Will look to include this in the next minor release.
connects #121
This isn't a full implementation of a OCI registry client at this point, and doesn't support auth etc. Only intended as a last resort fallback for now, and as such I've not wired it in to be selectable by host via command line flags.