Skip to content
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

Merged
merged 2 commits into from
Jul 12, 2022
Merged

Introduce registry package #276

merged 2 commits into from
Jul 12, 2022

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Jun 21, 2022

internal/registry package consolidates all the registry provider logins.
The internal/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.
internal/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.

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 be
easier to fix any potential issues if it's in the same repo with all the
integration tests.

Part of #264

@darkowlzz
Copy link
Contributor Author

darkowlzz commented Jun 29, 2022

@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.

@pjbgf pjbgf added this to the GA milestone Jul 7, 2022
@darkowlzz darkowlzz force-pushed the registry-login branch 2 times, most recently from b1f7260 to f95a6d7 Compare July 11, 2022 12:44
@darkowlzz
Copy link
Contributor Author

I've made some changes based on the review comments and tested it using the test automation.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @darkowlzz 🏅

Copy link
Member

@pjbgf pjbgf left a 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 <darkowlzz@protonmail.com>
Signed-off-by: Sunny <darkowlzz@protonmail.com>
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Merci @darkowlzz 🙇

@darkowlzz darkowlzz merged commit 3f684a0 into main Jul 12, 2022
@darkowlzz darkowlzz deleted the registry-login branch July 12, 2022 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants