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

feat: logout docker registries in post step #70

Merged
merged 3 commits into from
Aug 9, 2020

Conversation

clareliguori
Copy link
Member

In the post-job cleanup, do a docker logout for all registries that had a successful docker login via this action.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@clareliguori clareliguori requested review from a team, piradeepk and allisaurus and removed request for a team, piradeepk and allisaurus July 29, 2020 23:22
@clareliguori clareliguori requested review from piradeepk and removed request for a team August 4, 2020 23:04
Copy link
Contributor

@piradeepk piradeepk left a comment

Choose a reason for hiding this comment

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

Looks good!

cleanup.js Outdated
Comment on lines 34 to 37
if (exitCode != 0) {
core.debug(doLogoutStdout);
throw new Error('Could not logout: ' + doLogoutStderr);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make more sense to try to log out of all of the registries and then throw an error with which ones failed instead of failing after the first?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup that makes sense, fixed in latest commit

Copy link
Contributor

@piradeepk piradeepk left a comment

Choose a reason for hiding this comment

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

Looks great! Ship it! 💯

@mergify mergify bot merged commit 6cfbb32 into aws-actions:master Aug 9, 2020
@rubenfonseca
Copy link

Due to this change, our CI pipeline broke. We have multiple workers on the same machine (same docker daemon) and now while they are pushing things to docker, as soon as one job finishes, it will wipe the credentials for the other jobs. Is there a way to make this behaviour optional?

@allisaurus
Copy link
Contributor

@rubenfonseca Thanks for reporting this, I'll cut and issue and we'll work on a fix.

@allisaurus
Copy link
Contributor

@rubenfonseca in the meantime you can pin to a previous version of the action to get unblocked. See #77

@rubenfonseca
Copy link

rubenfonseca commented Aug 13, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants