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

Build Docker image and push to GHCR #230

Open
wants to merge 19 commits into
base: unstable/v1
Choose a base branch
from

Conversation

br3ndonland
Copy link
Contributor

@br3ndonland br3ndonland commented Apr 19, 2024

Description

Closes #58

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

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.

Related

@webknjaz
Copy link
Member

This looks.. intriguing! I don't remember if I ever considered combining composite+docker actions (I did play with having two composites in the same repo in the past, though).

I'll need to take some time to think about it and look through the patch more closely. Please, don't expect an immediate review, however it does look very promising at glance!

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!

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

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.

webknjaz pushed a commit to br3ndonland/gh-action-pypi-publish that referenced this pull request May 16, 2024
@webknjaz
Copy link
Member

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.

pull_request:
workflow_run:
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will return to this suggestion at a later time.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should be able to circle back to this already...

Copy link
Contributor Author

@br3ndonland br3ndonland Sep 8, 2024

Choose a reason for hiding this comment

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

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?

@webknjaz I removed workflow_call and switched to a reusable workflow instead.

Note that the required status checks for the repo will need to be updated.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
steps:
- name: Reset path if needed
run: |
# Reset path if needed
Copy link
Member

Choose a reason for hiding this comment

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

Why would this be needed outside the container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Show resolved Hide resolved
@webknjaz
Copy link
Member

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
Copy link
Contributor Author

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

@webknjaz
Copy link
Member

Commits like 213c885ac41d769527ac150e2e633bb1ccd886d4 aren't really necessary since Git would automatically absorb changes applied separately. FYI. In fact, this one may have a harmful effect — when the label change is merged into the default branch, and this one rebased on top, Git will keep the removal commit and attempt deleting the label 🤷‍♂️

Anyway, we'll address this later on.

create-docker-action.py Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
create-docker-action.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

@br3ndonland I'm sure my review is incomplete, but hopefully it gives you enough ideas to try out until the next time.

I think it'd be nice to get this in before #236 if at all possible.

@webknjaz
Copy link
Member

@br3ndonland could you also rebase this branch locally? The GH button doesn't work, meaning there's going to be some conflicts to resolve.

if: runner.os != 'Linux'
run: |
>&2 echo This action is only able to run under GNU/Linux environments
exit 1
Copy link
Member

Choose a reason for hiding this comment

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

Could you add CI jobs testing that this fails under corresponding envs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added CI jobs that test failure on macOS and Windows.

Comment on lines 11 to 12
tag:
description: Docker image tag
Copy link
Member

Choose a reason for hiding this comment

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

Can we incorporate the prior art from the discussions in #45 and accept the 3-segment version as an input, then deduce everything else from that + push the Git tag and advance the proper branches? Perhaps, this could also happen before this PR so it exists as a separate base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we incorporate the prior art from the discussions in #45 and accept the 3-segment version as an input, then deduce everything else from that + push the Git tag and advance the proper branches? Perhaps, this could also happen before this PR so it exists as a separate base.

I don't understand what you're asking here. You want separate inputs for major, minor, and patch versions? What if you're building the Docker image from the release/v1 branch? What is the "3-segment version" in that case?

Copy link
Member

Choose a reason for hiding this comment

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

@br3ndonland I was talking about the workflow inputs. Everything else should be computed from them.

The current workflow for me is as follows:

  1. Tag v1.10.1 on top of unstable/v1
  2. Fast-forward release/v1 to v1.10.1
  3. Fast-forward release/v1.10 to v1.10.1 (or create it)
  4. git push --atomic everything in one go
  5. Create a GitHub Release from the UI

With this, my 3-segment “input” is 1.10.1 and v1+v1.10 are being extracted from that. Do you think we could bake containers into this flow with not many changes? This will involve tagging the same image with multiple tags, as I understand. And the end-users referencing versions by both tags and branches should be able to retrieve whatever container there is, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@webknjaz thanks for explaining that. I think we're actually pretty close to the desired workflow. There's just a small update needed to the Docker build workflow so it pushes tags with the major and minor version numbers. I've pushed that change.

Your updated workflow would look like this:

  1. Tag v1.10.1 on top of unstable/v1
  2. Fast-forward release/v1 to v1.10.1
  3. Fast-forward release/v1.10 to v1.10.1 (or create it)
  4. git push --atomic everything in one go
  5. Trigger the 🏗️ workflow from the Git tag. The workflow will push the Docker image with three Docker tags:
    1. ghcr.io/pypa/gh-action-pypi-publish:v1.10.1
    2. ghcr.io/pypa/gh-action-pypi-publish:v1.10
    3. ghcr.io/pypa/gh-action-pypi-publish:v1
  6. Create a GitHub Release from the UI

br3ndonland added a commit to br3ndonland/gh-action-pypi-publish that referenced this pull request Jul 10, 2024
br3ndonland added a commit to br3ndonland/gh-action-pypi-publish that referenced this pull request Jul 10, 2024
pypa#230 (comment)

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
br3ndonland added a commit to br3ndonland/fastenv that referenced this pull request Jul 14, 2024
This project uses pypa/gh-action-pypi-publish to publish Python packages
to PyPI with an OIDC trusted publisher (6e532c6).

pypa/gh-action-pypi-publish is 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 will
hopefully change in the near future if Docker images are pre-built and
pushed to a registry (pypa/gh-action-pypi-publish#230). In the meantime,
this commit will move related steps to a dedicated GitHub Actions job so
that the Docker image is not built every time GitHub Actions jobs run.

6e532c6
https://docs.github.com/en/actions/creating-actions/creating-a-docker-container-action
https://docs.pypi.org/trusted-publishers/
https://github.com/pypa/gh-action-pypi-publish
br3ndonland added a commit to br3ndonland/inboard that referenced this pull request Jul 14, 2024
This project uses pypa/gh-action-pypi-publish to publish Python packages
to PyPI with an OIDC trusted publisher (59ec546).

pypa/gh-action-pypi-publish is 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 will
hopefully change in the near future if Docker images are pre-built and
pushed to a registry (pypa/gh-action-pypi-publish#230). In the meantime,
this commit will move related steps to a dedicated GitHub Actions job so
that the Docker image is not built every time GitHub Actions jobs run.

59ec546
https://docs.github.com/en/actions/creating-actions/creating-a-docker-container-action
https://docs.pypi.org/trusted-publishers/
https://github.com/pypa/gh-action-pypi-publish
@webknjaz
Copy link
Member

webknjaz commented Sep 4, 2024

@br3ndonland this also needs to be rebased now that the attestations PR is in.

br3ndonland and others added 17 commits September 7, 2024 13:13
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)

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
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.

Build&publish the base container to GHCR + point to it from action
3 participants