Build Docker image and push to GHCR #230

br3ndonland · 2024-04-19T01:51:01Z

Description

Closes #58

Up to this point, the project has been set up as a Docker action referencing the Dockerfile.

Lines 86 to 88 in 3fbcf7c

    
           runs: 
        
             using: docker 
        
             image: Dockerfile

The downside to using the Dockerfile for the action is that the Docker image must be built every time the action is used (#58).

This PR will set up the project to build the Docker image and push it to GitHub Container Registry (GHCR). This change will speed up user workflows every time the action is used because the workflows will simply pull the Docker image from GHCR instead of building again.

Changes

Build container image with GitHub Actions

This PR will build Docker images with the Docker CLI (docker build). Builds will include inline cache metadata so layers can be reused by future builds.

This PR only proposes to build container images for x86_64 (linux/amd64) because GitHub Actions Linux runners currently only support x86_64 CPU architectures (actions/runner-images#5631), and this project only supports GitHub Actions Linux runners. The README explains:

Since this GitHub Action is docker-based, it can only be used from within GNU/Linux based jobs in GitHub Actions CI/CD workflows. This is by design and is unlikely to change due to a number of considerations we rely on.

Push container image to GHCR

The workflow will log in to GHCR using the built-in GitHub token and push the Docker image. Workflow runs triggered by pull requests will build the Docker image and run the smoke tests but will not push the Docker image.

Update action to pull container image from GHCR

Docker actions support pulling in pre-built Docker images by supplying a registry address to the image: key. The downside to this syntax is that there's no way to specify the correct Docker tag because the GitHub Actions image: and uses: keys don't accept any context. For example, if a user's workflow has uses: pypa/gh-action-pypi-publish@release/v1.8, then the action should pull in a Docker image built from the release/v1.8 ref, something like ghcr.io/pypa/gh-action-pypi-publish:release-v1.8 (Docker tags can't have /).

# this works but the image tag can't be customized
runs:
  using: docker
  image: docker://ghcr.io/pypa/gh-action-pypi-publish:release-v1.8

# this doesn't work because `image:` doesn't support context
runs:
  using: docker
  image: docker://ghcr.io/pypa/gh-action-pypi-publish:${{ github.action_ref }}

The workaround is to switch the top-level action.yml to a composite action that then calls the Docker action, substituting the correct image name and tag.

Originally I thought that I'd have a workflow where I trigger a release, that release adds a commit that hardcodes an update to action.yml with the "future" version tag, tags that commit and pushes it (post docker publish). It wouldn't be on the main branch, the tags would be on the orphaned leaves.

This looks like a better idea so far. Thanks again!

pypa#230 (comment)

br3ndonland · 2024-04-26T23:12:16Z

That sounds great. Take your time. Thanks for your consideration.

If you do decide to accept this change, I'm happy to help maintain the workflows in the future. Feel free to mention me @br3ndonland and I will help address any issues that come up.

Up to this point, the project has been set up as a Docker action referencing the Dockerfile. The downside to using the Dockerfile for the action is that the Docker image must be built every time the action is used. This commit will set up the project to build the Docker image and push it to GitHub Container Registry (GHCR). This change will speed up user workflows every time the action is used because the workflows will simply pull the Docker image from GHCR instead of building again. Changes: - Add required metadata to Dockerfile - Build container image with GitHub Actions - Push container image to GHCR Docker actions support pulling in pre-built Docker images. The downside is that there's no way to specify the correct Docker tag because the GitHub Actions `image` and `uses:` keys don't accept any context. For example, if a user's workflow has `uses: pypa/gh-action-pypi-publish@release/v1.8`, then the action should pull in a Docker image built from the `release/v1.8` branch, something like `ghcr.io/pypa/gh-action-pypi-publish:release-v1.8` (Docker tags can't have `/`). The workaround is to switch the top-level `action.yml` to a composite action that then calls the Docker action, substituting the correct image name and tag.

pypa#230 (comment)

webknjaz · 2024-05-16T15:56:33Z

Thanks! I've hit "rebase" on the UI to get this on top of the recent changes/linting/lockfile bumps but haven't yet looked into it deeper.

webknjaz · 2024-05-16T15:59:49Z

.github/workflows/build-and-push-docker-image.yml

+          --tag $IMAGE
+    - name: Log in to GHCR
+      if: github.event_name != 'pull_request'
+      run: |


I usually prefer this syntax so that the whole thing is parsed as a single line:

Suggested change

run: |

run: >-

Generally I don't use run: >- because it will fold the contents of the block into a single line. This can alter the behavior of the script within the block. On this line though, it makes no difference, so sure.

webknjaz · 2024-05-16T19:10:30Z

.github/actions/run-docker-container/action.yml

+  args:
+  - ${{ inputs.user }}
+  - ${{ inputs.password }}
+  - ${{ inputs.repository-url }}
+  - ${{ inputs.packages-dir }}
+  - ${{ inputs.verify-metadata }}
+  - ${{ inputs.skip-existing }}
+  - ${{ inputs.verbose }}
+  - ${{ inputs.print-hash }}


These aren't actually used. Somebody contributed them, but I don't see them being needed. So perhaps we shouldn't keep them around anymore.

Aren't the inputs used by the entrypoint script?

gh-action-pypi-publish/twine-upload.sh

Lines 37 to 41 in 699cd61

INPUT_REPOSITORY_URL="$(get-normalized-input 'repository-url')"

INPUT_PACKAGES_DIR="$(get-normalized-input 'packages-dir')"

INPUT_VERIFY_METADATA="$(get-normalized-input 'verify-metadata')"

INPUT_SKIP_EXISTING="$(get-normalized-input 'skip-existing')"

INPUT_PRINT_HASH="$(get-normalized-input 'print-hash')"

webknjaz · 2024-05-16T19:15:56Z

.github/workflows/build-and-push-docker-image.yml

+  push:
+    branches: ["release/*", "unstable/*"]
+    tags: ["*"]


When we push the tag first, there's going to be some time when the new version is out already, but the container doesn't actually exist yet. Therefore, I prefer building my release automation processes around the workflow_dispatch trigger, so that I'm able to type in the target version in the UI, get the image published and only then push a tag, only when it's known that the image exists already. Let's try reworking this with similar philosophy in mind.

Thanks for clarifying. I've replaced the tags trigger with workflow_dispatch instead (049447a).

webknjaz · 2024-05-16T19:18:18Z

.github/workflows/self-smoke-test-action.yml

  pull_request:
+  workflow_run:


Two separate workflow runs is often hard to track. Instead, I adopted a practice of modularizing the workflow pieces as reusable workflows having the reusable- prefix in their names. This allows embedding everything in all the right places. Let's try this, WDYT?

I will return to this suggestion at a later time.

webknjaz · 2024-05-16T19:20:53Z

.github/workflows/self-smoke-test-action.yml

-      uses: actions/checkout@v3
-      with:
-        path: test
+      uses: actions/checkout@v4


Can we not make version upgrades in the same PR? And I'd rather not change the testing logic/dirs unless it's required somehow.

Ok, I'll stick with actions/checkout@v3 and keep the previous test path.

webknjaz · 2024-05-16T19:22:13Z

.pre-commit-config.yaml

@@ -31,7 +31,7 @@ repos:
    args:
    - --builtin-schema
    - github-workflows-require-timeout
-    files: ^\.github/workflows/[^/]+$
+    files: ^\.github\/workflows/[^/]+$


Could you submit this separately?

I've removed this change. The existing regex (^\.github/workflows/[^/]+$) seems like it should work fine, but pre-commit was matching files in .github/actions/ and raising errors. Dynamically generating the entire action.yml as you suggested will avoid the problem.

webknjaz · 2024-05-16T19:23:03Z

Dockerfile

@@ -3,6 +3,7 @@ FROM python:3.12-slim
 LABEL "maintainer" "Sviatoslav Sydorenko <wk+pypa@sydorenko.org.ua>"
 LABEL "repository" "https://github.com/pypa/gh-action-pypi-publish"
 LABEL "homepage" "https://github.com/pypa/gh-action-pypi-publish"
+LABEL "org.opencontainers.image.source" "https://github.com/pypa/gh-action-pypi-publish"


I'd accept this in a separate PR even before the rest is figured out/refactored.

Moved to separate PR (#241).

webknjaz · 2024-05-16T19:37:27Z

action.yml

+  steps:
+  - name: Reset path if needed
+    run: |
+      # Reset path if needed


Why would this be needed outside the container?

Because you've set up a test that modifies the $PATH (#112, 1350b8b).

gh-action-pypi-publish/.github/workflows/self-smoke-test-action.yml

Lines 85 to 90 in 699cd61

- name: ✅ Smoke-test the locally checked out action

uses: ./test

env:

DEBUG: >-

true

PATH: utter-nonsense

webknjaz · 2024-05-16T19:40:33Z

action.yml

+      REF=${{ env.ACTION_REF || github.ref_name }}
+      REPO=${{ env.ACTION_REPO || github.repository }}
+      echo "ref=$REF" >>"$GITHUB_OUTPUT"
+      echo "repo=$REPO" >>"$GITHUB_OUTPUT"
+    shell: bash
+    env:
+      ACTION_REF: ${{ github.action_ref }}
+      ACTION_REPO: ${{ github.action_repository }}


Isn't it easier to do it like this?

Suggested change

REF=${{ env.ACTION_REF || github.ref_name }}

REPO=${{ env.ACTION_REPO || github.repository }}

echo "ref=$REF" >>"$GITHUB_OUTPUT"

echo "repo=$REPO" >>"$GITHUB_OUTPUT"

shell: bash

env:

ACTION_REF: ${{ github.action_ref }}

ACTION_REPO: ${{ github.action_repository }}

REF=${{ github.action_ref || github.ref_name }}

REPO=${{ github.action_repository || github.repository }}

echo "ref=${REF}" >>"${GITHUB_OUTPUT}"

echo "repo=${REPO}" >>"${GITHUB_OUTPUT}"

shell: bash

You would think so, but the syntax is needed to work around a limitation of composite actions. See:

actions/runner#2473
github/docs#25336 (comment)

webknjaz · 2024-05-16T19:45:43Z

action.yml

+  - name: Set Docker image name and tag
+    run: |
+      # Set Docker image name and tag
+      # if action run was triggered by a pull request to this repo,
+      # build image from Dockerfile because it has not been pushed to GHCR,
+      # else pull image from GHCR
+      if [[ $GITHUB_EVENT_NAME == "pull_request" ]] &&
+        [[ $GITHUB_REPOSITORY == "pypa/gh-action-pypi-publish" ]]; then
+        IMAGE="../../../Dockerfile"
+      else
+        REF=${{ steps.set-repo-and-ref.outputs.ref }}
+        REPO=${{ steps.set-repo-and-ref.outputs.repo }}
+        IMAGE="docker://ghcr.io/$REPO:${REF/'/'/'-'}"
+      fi
+      FILE=".github/actions/run-docker-container/action.yml"
+      sed -i -e "s|{{image}}|$IMAGE|g" "$FILE"
+    shell: bash


So I was thinking… Why do we need to check in action.yml to Git even if this modifies it anyway? Why not generate it all, then?

Also, I'd use Python instead of Bash here. Then, it'd be possible to have a dict with data and write it as YAML using json.dump() (because JSON is valid YAML, almost always).

That could work. I've added a Python script that generates the Docker action (9ae1850).

Regarding your related comment:

I think that generating the file is a good idea. It should be possible to write the file without bringing in the PyYAML dependency. But it's not that easy for reading it. Can we make use of yq somehow, and convert YAML to JSON this way, maybe?

I've started by using PyYAML but will think about a way to do this with yq.

.github/actions/run-docker-container/action.yml

webknjaz · 2024-05-16T19:50:43Z

action.yml

+      user: ${{ inputs.user }}
+      password: ${{ inputs.password }}
+      repository-url: ${{ inputs.repository-url || inputs.repository_url }}
+      packages-dir: ${{ inputs.packages-dir || inputs.packages_dir }}


This would break the deprecation messages, it seems. Have you checked?

I don't think so. The new inputs don't have defaults, so for example, ${{ inputs.repository-url || inputs.repository_url }} will default to inputs.repository_url and deprecationMessage will be logged.

webknjaz · 2024-05-16T20:01:27Z

I'm done with the initial review. More is needed, but I'd rather accept what I can through separate PRs to make this one smaller. And the suggested refactoring could be done in parallel. I think that generating the file is a good idea. It should be possible to write the file without bringing in the PyYAML dependency. But it's not that easy for reading it. Can we make use of yq somehow, and convert YAML to JSON this way, maybe?

br3ndonland · 2024-05-27T20:43:49Z

@webknjaz thank you for your detailed review. I've addressed most of your comments so far.

webknjaz reviewed Apr 25, 2024

View reviewed changes

.github/workflows/build-and-push-docker-image.yml Show resolved Hide resolved

webknjaz mentioned this pull request Apr 26, 2024

[Roadmap] Implement PEP 740 pypi/warehouse#15871

Open

16 tasks

br3ndonland added a commit to br3ndonland/gh-action-pypi-publish that referenced this pull request Apr 26, 2024

Separate docker login and docker push

bb0315f

pypa#230 (comment)

br3ndonland added 3 commits May 16, 2024 11:55

Fix pre-commit errors

769f499

Separate docker login and docker push

3384fad

pypa#230 (comment)

webknjaz force-pushed the ghcr branch from bb0315f to 3384fad Compare May 16, 2024 15:55

webknjaz reviewed May 16, 2024

View reviewed changes

.github/actions/run-docker-container/action.yml Outdated Show resolved Hide resolved

webknjaz reviewed May 16, 2024

View reviewed changes

br3ndonland added 7 commits May 27, 2024 16:03

Generate Docker container action with Python

9ae1850

Reset pre-commit files: regex

1b535ae

Remove added Docker LABEL

213c885

Use YAML block strip syntax (>-) where possible

5f0743b

Add workflow_dispatch trigger for Docker builds

049447a

Don't update actions/checkout@v3

2fb692b

Reset smoke test path

30c8fae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build Docker image and push to GHCR #230

Build Docker image and push to GHCR #230

br3ndonland commented Apr 19, 2024 •

edited

webknjaz commented Apr 25, 2024

br3ndonland commented Apr 26, 2024

webknjaz commented May 16, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024 •

edited

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz May 16, 2024

br3ndonland May 27, 2024 •

edited

webknjaz May 16, 2024

br3ndonland May 27, 2024

webknjaz commented May 16, 2024

br3ndonland commented May 27, 2024

	INPUT_REPOSITORY_URL="$(get-normalized-input 'repository-url')"
	INPUT_PACKAGES_DIR="$(get-normalized-input 'packages-dir')"
	INPUT_VERIFY_METADATA="$(get-normalized-input 'verify-metadata')"
	INPUT_SKIP_EXISTING="$(get-normalized-input 'skip-existing')"
	INPUT_PRINT_HASH="$(get-normalized-input 'print-hash')"

	- name: ✅ Smoke-test the locally checked out action
	uses: ./test
	env:
	DEBUG: >-
	true
	PATH: utter-nonsense

Build Docker image and push to GHCR #230

Are you sure you want to change the base?

Build Docker image and push to GHCR #230

Conversation

br3ndonland commented Apr 19, 2024 • edited

Description

Changes

Build container image with GitHub Actions

Push container image to GHCR

Update action to pull container image from GHCR

Related

webknjaz commented Apr 25, 2024

br3ndonland commented Apr 26, 2024

webknjaz commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

br3ndonland May 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

br3ndonland May 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented May 16, 2024

br3ndonland commented May 27, 2024

br3ndonland commented Apr 19, 2024 •

edited

br3ndonland May 27, 2024 •

edited

br3ndonland May 27, 2024 •

edited