Implement both the Add and Delete operations as no-ops #847
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These two operations are called when docker tries to store and delete credentials (docker login and docker logout) respectively. This change makes it so that both of these operations are implemented as nops instead of returning a "not implemented" error. The goal here is to ensure compatibilities with applications and tools that call docker login or docker logout for the user as part of their normal operations.
Co-authored-by: Gavin Inglis <43075615+ginglis13@users.noreply.github.com>
jamestelfer
reviewed
Jul 31, 2024
jamestelfer
reviewed
Jul 31, 2024
therealvio
changed the title
…login With the new capability to opt-out of saving credentials, we can guide users to not to save ecr credentials by setting the AWS_ECR_IGNORE_CREDS_STORAGE environment variable when encountering the `notImplemented` error. Ref: https://docs.aws.amazon.com/cli/latest/reference/ecr/get-login-password.html
jamestelfer
approved these changes
Jul 31, 2024
There was a problem hiding this comment.
LGTM @therealvio, thanks for picking this up for us! (Context, both @therealvio and I work at Culture Amp.)
Hopefully @swagatbora90 is happy with this too and we can improve our CI experience once this is released.
jamestelfer
reviewed
Aug 1, 2024
This change fixes an incorrect use of the test's scope. The parent was used for assertions and setting the env. This now should be using the correct instance of the `*testing.T` for the test.
therealvio
force-pushed
the
creds-add-delete-nop
branch
from
3cb4c16
to
d551297
Compare
swagatbora90
approved these changes
Aug 5, 2024
austinvazquez
approved these changes
Aug 5, 2024
|
Approvals are in, and checks have passed. What do we need to do next to get this merged? |
|
Done thanks @therealvio for pull this over the line. |
|
Thanks @austinvazquez, @swagatbora90 and @jamestelfer for your assistance on this 🙇 . I will mention the merge in the original thread for the original onlookers. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #102 & #154
Description of changes:
(Revive of #315)
Currently, both the Add and Delete operations in this helper return a not implemented error. This can lead to issues where docker login and docker logout stop working for the user while displaying a confusing not implemented error.
Following the suggestion in #154 (comment), this PR adds an environment variable (AWS_ECR_IGNORE_CREDS_STORAGE) to optionally ignore these requests and return no error. When AWS_ECR_IGNORE_CREDS_STORAGE=true, instead of returning a not implemented error, we return nil.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.