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

chore(deps): use docker/login-action consistently instead of Azure/docker-login #12791

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

agilgur5
Copy link
Member

Follow-up to #12775

Motivation

Modifications

Verification

This only runs during releases, so a bit hard to test 😕

…/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 <[email protected]>
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies github_actions Pull requests that update Github_actions dependencies area/build Build or GithubAction/CI issues labels Mar 12, 2024
Copy link
Member

@terrytangyuan terrytangyuan left a 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?

@agilgur5
Copy link
Member Author

agilgur5 commented Mar 26, 2024

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.

@agilgur5
Copy link
Member Author

Merged main for new PR checks per #13027

@terrytangyuan terrytangyuan merged commit 64850e0 into argoproj:main Jun 7, 2024
16 checks passed
@agilgur5 agilgur5 deleted the chore-deps-docker-login branch June 7, 2024 03:38
@agilgur5
Copy link
Member Author

agilgur5 commented Jun 8, 2024

Huh so this did partially fail on release on main:

Run echo $(jq -c '. + { "experimental": "enabled" }' ${DOCKER_CONFIG}/config.json) > ${DOCKER_CONFIG}/config.json
jq: error: Could not open file /config.json: No such file or directory
/home/runner/work/_temp/3b2a11f7-f9e3-4c1e-8ab2-431101712831.sh: line 1: /config.json: Permission denied

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 ${DOCKER_CONFIG} is empty per the error message 🤔 So it may have only been set by Azure/docker-login, which means the code was relying on an env var side effect, if I'm understanding correctly.
The default is ~/.docker/config.json so this may be easily replaceable

I wish the release GHA workflow was easier to test though 😕

@agilgur5
Copy link
Member Author

agilgur5 commented Jun 8, 2024

The default is ~/.docker/config.json so this may be easily replaceable

Added this as a default in #13155

@agilgur5 agilgur5 added this to the v3.5.x patches milestone Jul 30, 2024
agilgur5 added a commit that referenced this pull request Jul 30, 2024
…/docker-login` (#12791)

Signed-off-by: Anton Gilgur <[email protected]>
(cherry picked from commit 64850e0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues github_actions Pull requests that update Github_actions dependencies type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants