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

feat: add zizmor Docker image #319

Closed
wants to merge 7 commits into from

Conversation

trallard
Copy link
Contributor

@trallard trallard commented Dec 17, 2024

Closes #135

I am opening a WIP PR since I have some outstanding questions.

Notes 📝

  • I had originally suggested basing this on the uv Docker image. However, I think that might be overkill for a "vanilla" image for Zizmor, so I used the Rust image instead (bookworm-slim, which also helps keep the image quite small). That way, we provide the minimal required amount and leave it to users to build off as they need.

Questions/needs input 💬

  • I set the entry point as ENTRYPOINT ["/bin/bash"], or would it be best to point it to Zizmor directly? ✅
  • For building and pushing the images:
    • I generally favour pushing images as part of my release processes, so I plan to add a reusable workflow that can be called from the release.yml workflow. ✅
    • I think it would be suitable to build both linux/amd64 and linux/arm64 images.
  • Also, it seems suitable to add a Docker entry to the zizmor docs (similar to https://docs.astral.sh/uv/guides/integration/docker/)

TODO for @woodruffw:

  • Create a docker environment for the build and publish Docker job
  • Add relevant credentials to authenticate to GHCR

@trallard trallard changed the title feat: add Dockerfile for zizmor feat: add zizmor Docker image Dec 17, 2024
@woodruffw
Copy link
Owner

Thanks @trallard, I appreciate you opening this!

  • However, I think that might be overkill for a "vanilla" image for Zizmor, so I used the Rust image instead (bookworm-slim, which also helps keep the image quite small). That way, we provide the minimal required amount and leave it to users to build off as they need.

Good call IMO 🙂 -- however, I wonder if we could keep the PyPI installation (i.e. via uv tool install or pip install rather than rebuilding with cargo install. I think this would keep our CI fast and simplify the arm64/amd64 image builds (since we already have native builds for both). WDYT?

  • I set the entry point as ENTRYPOINT ["/bin/bash"], or would it be best to point it to Zizmor directly?

Pointing to zizmor directly makes sense to me!

  • I generally favour pushing images as part of my release processes, so I plan to add a reusable workflow that can be called from the release.yml workflow.
  • I think it would be suitable to build both linux/amd64 and linux/arm64 images.

SGTM -- either a reusable workflow or a new docker job within the existing workflow would be fine by me. Dealer's choice 🙂

Yep! Ideally this PR would include those doc changes, although they could also be a follow-up.

@trallard
Copy link
Contributor Author

Yay! Thanks for the quick follow up.

These all seem good so I will work the rest of the things and can add some docs in this PR, then I can iterate if needed.

Good call IMO 🙂 -- however, I wonder if we could keep the PyPI installation (i.e. via uv tool install or pip install rather than rebuilding with cargo install. I think this would keep our CI fast and simplify the arm64/amd64 image builds (since we already have native builds for both). WDYT?

I can certainly do that. Should be easy enough to swap.

@woodruffw
Copy link
Owner

Hey @trallard! Do you intend to pick this up sometime soon? No problem at all if not, but if you think it's gonna be >1 month I'm happy to build on top of what you've got here so I can land this soon-ish 🙂

@trallard
Copy link
Contributor Author

Hey there I took some time off but should be able to pick this in the next week or so.

@tyler36
Copy link

tyler36 commented Feb 4, 2025

Will this be an official docker image? Are you also planning to add this image Docker Hub?

@woodruffw
Copy link
Owner

Will this be an official docker image?

It'll be official in the sense that it comes from this repo, so as official as the cargo install and pip install sources. Like the other sources, you can expect it to receive updates on the same cadence/schedule as the main repository releases.

Are you also planning to add this image Docker Hub?

It'll be available on GHCR most likely, since GHCR is easy to publish to with GitHub Actions.

@trallard trallard marked this pull request as ready for review February 4, 2025 15:53
@trallard
Copy link
Contributor Author

trallard commented Feb 4, 2025

Now that I have finally caught up with my massive backlog, I can wrap this up so I am taking this out of Draft mode.

To test locally (i.e., build the Docker image), one can run the following command with a valid zizmor version:

docker build \
    --build-arg ZIZMOR_VERSION=1.3.0 \
    -t zizmor:latest .

Since I ended up using uv to install zizmor it seemed that the easiest way to add a GH workflow was to add it at the end of the pypi workflow to ensure the release exists and we can install zizmor properly.

If we are happy with this as is I can add some docs right away.

Copy link
Owner

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Thanks @trallard! I think I'd prefer having the Docker build be in its own workflow, for reasons in the review comments below. But I'm curious to hear what you think 🙂

name: docker
if: ${{ startsWith(github.ref, 'refs/tags/') || github.event_name == 'workflow_dispatch' }}
# Only run if the release job was successfully completed
needs: [release]
Copy link
Owner

Choose a reason for hiding this comment

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

There might be a bit of a race condition with this, sadly: PyPI's propagation can take a few minutes IME, so running immediately after a release might fail due to the index not listing the release yet.

I think there are two (easy-ish) options here:

  1. Add some kind of blocking "wait for PyPI" job so that the flow goes release -> wait-for-pypi -> docker-build instead
  2. Move this to a separate docker-publish.yml or similar that can be triggered separately, e.g. by a workflow_dispatch with a tag: input

I'd be OK with either of these but a slight preference for (2), especially since we can give it a cron trigger that'll do the update automatically 🙂

Copy link
Contributor Author

@trallard trallard Feb 4, 2025

Choose a reason for hiding this comment

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

I was initially going for 2-ish, but then I realized the trigger would need to be other than a release tag so that this did not end up running before the PyPI release was ready, so I moved this inside the PyPI workflow, though I completely forgot about the potential delays on the PyPI side.

Let me move this out into a separate workflow again, as in 2) and we can sort out the trigger event (cron or something else).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative could be using the built wheels (already uploaded to the artefacts) to install zizmor (thus avoiding any potential delays on the PyPI side of things) so all the workflows would trigger from the same release tag event.

Copy link
Owner

Choose a reason for hiding this comment

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

That could work, although in that case we'd need to reimplement the wheel/tag matching, right? I think a separate workflow + trigger is good for now 🙂

@tyler36
Copy link

tyler36 commented Feb 5, 2025

Thank you for working on this. I'm excited to try it out!

@woodruffw
Copy link
Owner

I want to start experimenting with the image as a building block for an action, so I've opened #532 as a branch off of this. I'll likely merge that in a moment and iterate from there. Thanks a ton for your hard work here @trallard, I really appreciate it!

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.

Feature: Add a docker image
3 participants