-
Notifications
You must be signed in to change notification settings - Fork 86
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] Cache Login credentials #776
Conversation
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 looks great, thanks!
PS. The cache pkg branch is behind main and has old Kubernetes deps. Do you want to merge this into the cache-athn branch and rebase there?
a108669
to
c6ec0fb
Compare
fdefd65
to
d838d8a
Compare
d43d1bb
to
ad0d5fa
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.
We can test this in integration test by updating the test app and setting a cache instance in the ProviderOptions
in https://github.com/fluxcd/pkg/blob/oci/v0.37.1/oci/tests/integration/testapp/main.go#L50.
Signed-off-by: Soule BA <[email protected]>
I have triggered the integration tests on gcp: https://github.com/fluxcd/pkg/actions/runs/9561954422 |
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.
Thanks for adding the cache to the integration test.
Because the test app runs in the cluster in a separate pod and the test runner runs outside of the cluster, we don't have a nice way to also test the use of data in the cache. The test app can be updated to test this specific scenario to perform another login and check the obtained credential with the previous one. But I guess that's not needed as we test the same in unit tests.
Switch return type from *time.Time to time.Time. Signed-off-by: Soule BA <[email protected]>
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!
This PR add caching to
Login function
for eachProvider
.Depends on #766