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

pathogen-repo-ci: Log in to docker.io and ghcr.io if possible #38

Merged
merged 3 commits into from
May 8, 2023

Conversation

tsibley
Copy link
Member

@tsibley tsibley commented May 5, 2023

See commit messages.

Related-to: nextstrain/docker-base#148

Testing

  • Checks pass

@tsibley tsibley force-pushed the trs/pathogen-repo-ci/login-to-image-registries branch 2 times, most recently from ae31ddb to 85745d4 Compare May 5, 2023 21:20
This lifts low rate limits on image pulls.  However, calling workflows
must explicitly opt in with "secrets: inherit" in order for this
reusable workflow to be able to see the org-level secret containing the
token.

Related-to: <nextstrain/docker-base#148>
This allows the use of docker-base images we transiently stage at
ghcr.io before publishing to docker.io.  A new "permissions:" block with
"packages: read" restricts the ghcr.io access to read-only.  This
addition requires explicitly enumerating the rest of the required
permissions too, which is only "contents: read" for actions/checkout.

Related-to: <nextstrain/docker-base#148>
@tsibley tsibley force-pushed the trs/pathogen-repo-ci/login-to-image-registries branch from 85745d4 to 83e7441 Compare May 5, 2023 21:24
@tsibley
Copy link
Member Author

tsibley commented May 5, 2023

The annotations when one of the optional login steps fails are kinda annoying in the UI:

image

(screenshot from using this workflow in nextstrain/docker-base#148)

Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

Too bad about the annoying error outputs...Would it be too hacky to check if secrets.DOCKER_TOKEN_PUBLIC_READ_ONLY exists in a separate step before "Log in to docker.io"?

@tsibley
Copy link
Member Author

tsibley commented May 8, 2023

Hmm, yeah, that would work.

We could also ditch docker/login-action and instead call docker login for both registries directly (and conditionally) from a single step (plus an if: always() cleanup step that did docker logout for each registry).

@victorlin
Copy link
Member

We could also ditch docker/login-action and instead call docker login

Maybe this will help with nextstrain/docker-base#131...

…et isn't available

Even though a login that fails due to the missing secret doesn't affect
the workflow's execution status, it does clutter the workflow's summary
page with a bunch of noisy warnings.

Suggestion to do this pre-check from @joverlee521 in review.  If only we
could check for secrets directly in a step condition ("if: …").
@tsibley
Copy link
Member Author

tsibley commented May 8, 2023

Pushed 906908a to conditionalize the docker.io login on the presence of the secret, to avoid the annoying warnings. \o/

It works without the secret:

Screenshot from 2023-05-08 11-51-29

and with it:

image

@tsibley tsibley force-pushed the trs/pathogen-repo-ci/login-to-image-registries branch from 21d5297 to 906908a Compare May 8, 2023 18:52
@tsibley tsibley merged commit c0cffdb into master May 8, 2023
@tsibley tsibley deleted the trs/pathogen-repo-ci/login-to-image-registries branch May 8, 2023 18:53
Comment on lines +55 to +60
# Log in, if possible, to docker.io (Docker Hub), since authenticated
# requests get higher rate limits (e.g. for image pulls). Our org-level
# secret DOCKER_TOKEN_PUBLIC_READ_ONLY is available to all our public
# repos on GitHub but only available here to this reusable workflow when
# called with "secrets: inherit". On Docker Hub, the token is granted
# "public read-only" access.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for setting this up!

.github/workflows/pathogen-repo-ci.yaml Show resolved Hide resolved
tsibley added a commit that referenced this pull request May 10, 2023
@victorlin pointed out¹ that we could launder access to the secrets
context thru the step env context without the need for an extra step.

I avoid putting the secret itself into the environment (which only
increases its potential visibility) and put only its presence/absence
instead.

¹ <#38 (comment)>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants