-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore(deps): use docker/login-action
consistently instead of Azure/docker-login
#12791
Conversation
…/docker-login` - `Azure/docker-login` seems to have been the first one to have been added in 6dc04b3, then `docker/login-action` was added after in c8eed1f, but it was not consistent - [`Azure/docker-login`](https://github.com/Azure/docker-login) is not maintained much, with the [last release](https://github.com/Azure/docker-login/releases/tag/v1.0.1) ~1.5 years ago and no update to Node v20 as of yet, despite issues and PRs on it from a few months ago - so use [`docker/login-action`](https://github.com/docker/login-action), which is more maintained, more centralized, more commonly used, and not on EoL Node - [last release](https://github.com/docker/login-action/releases/tag/v3.0.0) a few months ago, [last commit](docker/login-action@566711b) a few weeks ago Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
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.
Why not combining with #12775?
Well, to begin with, that one was made a few days beforehand (both made 2+ weeks ago), and has now been merged earlier too. Otherwise there are purposes are different, per PR title and the "Motivation" section in each. That one is an update, this one is changing the action itself to a different one (which happens to be more updated), as well as the configuration for the new action. That also entails different risk levels (updating typically being less risky than changing a dep wholesale). Similar to mine and other maintainers' PRs in the past, they are also independent of each other (or "atomic" as another term maintainers have used) and so don't need to be tied together -- and it would be better practice not to do so. |
Merged |
Huh so this did partially fail on release on
All the builds and pushes of images worked, it's the manifest push that failed; i.e. this line, which I didn't change in the PR. It seems like I wish the release GHA workflow was easier to test though 😕 |
Added this as a default in #13155 |
Follow-up to #12775
Motivation
Azure/docker-login
seems to have been the first one to have been added in chore: Use GitHub Actions to build Docker Images to allow publishing Windows Images #3291, thendocker/login-action
was added after in chore(build): Pre-push temporary manifest with linux/amd64. Fixes #4062 #4127, but currently both are being used without consistencyAzure/docker-login
is not maintained much, with the last release ~1.5 years ago and no update to Node v20 as of yet, despite issues and PRs on it from a few months ago (Node 16 is deprecated, move to node 20 Azure/docker-login#63, Update to nodejs v20 Azure/docker-login#64)Modifications
docker/login-action
, which is more maintained, more centralized, more commonly used, and not on EoL NodeVerification
This only runs during releases, so a bit hard to test 😕