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: migrate to docker org #40

Merged
merged 4 commits into from
Dec 6, 2024
Merged

chore: migrate to docker org #40

merged 4 commits into from
Dec 6, 2024

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Sep 13, 2023

Similar to docker/login-action#3

Make this action official and move it to docker org: docker/setup-docker-action.

cc @chris-crone @thaJeztah @neersighted

@crazy-max crazy-max force-pushed the migrate branch 2 times, most recently from b488030 to 693b3fa Compare September 13, 2023 08:38
@thaJeztah
Copy link
Member

SGTM

perhaps for a follow-up, we may want to add a bit more details to the README to explain why someone would use this (because Docker is also available by default), so perhaps we should include some scenarios where this action could help (and scenarios where users can "keep it simple" and use the default).

@neersighted
Copy link
Member

There's several aspects of the existing code I'm not sure about; this is incomplete, but here's some that jump out:

  • The release manifest is updated manually, and contains the GH release URL which is not very useful/relevant.
    • There are too many manual steps/points of failure here; this should either leverage existing infrastructure for discovery, or we should improve that infrastructure.
  • The actual download URL is constructed on the fly from the version tag from the manifest, but also from a template.
    • What do you think about offering a manifest.json on download.docker.com/*/static to improve this?
  • Inline PowerShell is pretty hard to maintain, and we should think about a better setup/OOBE for the Windows binaries.
    • What if we included the setup (PowerShell) script with the Windows static binaries?

@crazy-max
Copy link
Member Author

The release manifest is updated manually, and contains the GH release URL which is not very useful/relevant.

This is automated by GHA (docker/actions-toolkit#166) but manually approved atm. We need this to avoid users being rate-limited by the GitHub API when checking releases anonymously so we just depend on raw.githubusercontent.com (see docker/buildx#1563).

But I agree that it would be better to rely on smth provided by download.docker.com.

What do you think about offering a manifest.json on download.docker.com/*/static to improve this?

Yes sounds good!

What if we included the setup (PowerShell) script with the Windows static binaries

Yes indeed that's pretty hacky atm in the toolkit for Windows. A contrib dir with scripts along the win binaries could work 👍.

@crazy-max
Copy link
Member Author

crazy-max commented Sep 14, 2023

perhaps for a follow-up, we may want to add a bit more details to the README to explain why someone would use this (because Docker is also available by default), so perhaps we should include some scenarios where this action could help (and scenarios where users can "keep it simple" and use the default).

Actions README are minimal and mostly technical. Use cases are on https://docs.docker.com/build/ci/github-actions/ but I agree we should have smth for this action there as well. This reminds me we should do smth for the metadata-action that has a pretty huge README.

@thaJeztah
Copy link
Member

Yeah, it's mostly that I want to avoid users starting to use this action when they don't need to .. at all. So if we could have a short description "this is why you should use it (or not!)" would probably be a nice thing to have

(not a blocker, obviously)

@crazy-max
Copy link
Member Author

Yeah, it's mostly that I want to avoid users starting to use this action when they don't need to .. at all. So if we could have a short description "this is why you should use it (or not!)" would probably be a nice thing to have

(not a blocker, obviously)

Added a note:

image

description: 'Set up Docker for use in GitHub Actions by downloading and installing a version of Docker CE'
author: 'crazy-max'
name: Docker Setup Docker
description: Set up Docker for use in GitHub Actions by downloading and installing a version of Docker CE
Copy link
Member

Choose a reason for hiding this comment

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

I know the plan was to phase out the "CE" (and "Community Edition") term, so wondering if we should update these for "Docker Engine" (and "Docker CLI"), but honestly, don't know who's deciding on these things nowadays

Copy link
Member

Choose a reason for hiding this comment

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

I'd find it much easier/preferred to keep the CE as people know what it is and it's proliferated everywhere. Community Edition and Desktop is not the worst thing ever, and we use Engine as a more Kleenex term.

@crazy-max crazy-max force-pushed the migrate branch 2 times, most recently from 5fa4599 to 43393cb Compare October 29, 2024 14:10
@crazy-max crazy-max requested a review from vvoland November 26, 2024 09:06
@crazy-max crazy-max marked this pull request as ready for review November 26, 2024 09:07
@crazy-max crazy-max merged commit ccdfeca into master Dec 6, 2024
28 checks passed
@crazy-max crazy-max deleted the migrate branch December 6, 2024 11:59
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