-
Notifications
You must be signed in to change notification settings - Fork 69
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
Introduce registry package #276
Conversation
deda65f
to
71755ba
Compare
@pjbgf thanks for reviewing the actual login code. Since we didn't had any automated tests before for the cloud providers, I tried to keep the existing login code as they were and wrap them in a new structure, just to ensure I don't break anything. Now that we have automated tests, I'll go through your comments and address them based on actual testing. |
b1f7260
to
f95a6d7
Compare
I've made some changes based on the review comments and tested it using the test automation. |
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
Thanks @darkowlzz 🏅
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.
fantastik @darkowlzz! 🔝🔝
LGTM
registry package consolidates all the registry provider logins. The registry/login package contains a login Manager which manages logins for all the providers. For testability, it provides methods to modify the provider client configurations. registry/{aws/azure/gcp} packages contain clients for logging into the respective registry. The client APIs are mostly similar across all the providers, except for the small details related to overriding certain configurations for testing purposes. Each of the providers have test coverage to solidify the expected behavior in different scenarios. Signed-off-by: Sunny <[email protected]>
Signed-off-by: Sunny <[email protected]>
f95a6d7
to
e61b563
Compare
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. Merci @darkowlzz 🙇
internal/registry
package consolidates all the registry provider logins.The
internal/registry/login
package contains a loginManager
whichmanages logins for all the providers. For testability, it provides methods to
modify the provider client configurations.
internal/registry/{aws/azure/gcp}
packages contain clients for logginginto the respective registry. The client APIs are mostly similar across all the
providers, except for the small details related to overriding certain
configurations for testing purposes. Each of the providers have test
coverage to solidify the expected behavior in different scenarios.
This depends on #275 for testing to ensure that the native registry login
tests work before and after these changes.
NOTE: I'd like to gain some confidence in this package, maybe an IRC
release with this package, before moving it to
fluxcd/pkg
repo. It'd beeasier to fix any potential issues if it's in the same repo with all the
integration tests.
Part of #264