-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add support for OCI registries #402
Conversation
@giantswarm/team-honeybadger PTAL. This is still pending a test, but the structure will overall stay the same. |
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.
Generally looks reasonable
helmregistry.ProvLayerMediaType, | ||
} | ||
|
||
var registryStore content.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.
Can we make the content.Registry
an optional value for the helmclient
client struct?
That way we make our live easier down the road with private catalogs and such.
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 makes a lot of sense, sure.
Tested locally, pulling the chart works. I have a question or two to ask on the team's forum regarding some future-proofing of the design. |
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.
Nice job 👍 and I like the comments adding extra context.
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 🚀
Towards giantswarm/roadmap#391
Checklist