Skip to content

Added default multi-platform builds for amd64 and arm64 docker images.#7

Merged
kron-spar merged 1 commit intomainfrom
action-updates
Mar 13, 2024
Merged

Added default multi-platform builds for amd64 and arm64 docker images.#7
kron-spar merged 1 commit intomainfrom
action-updates

Conversation

@kron-spar
Copy link
Copy Markdown
Contributor

The way this branch currently works is that there are two default platforms it will build; both ARM64 and AMD64 images. The platforms argument may be omitted from the parent job that calls this composite action and it will still generate those images.

@kron-spar kron-spar requested a review from aelkiss February 23, 2024 20:46
@aelkiss
Copy link
Copy Markdown
Member

aelkiss commented Feb 26, 2024

Let's see if we can get the github-script, checkout, & login actions updated so they don't give deprecation warnings.

@aelkiss
Copy link
Copy Markdown
Member

aelkiss commented Feb 26, 2024

For multi-platform builds let's see how well the caching works - if it's pretty quick to build both platforms assuming that only the repository (and not dependencies) have changed then I think it makes sense to enable by default

Copy link
Copy Markdown
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

Would like to see the dependencies updated but otherwise looks good, and we can see about caching speedups for multiplatform builds.

@kron-spar kron-spar requested a review from aelkiss February 28, 2024 15:56
Copy link
Copy Markdown
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

I took a look at the runs from https://github.com/hathitrust/ht_indexer/actions and there are a few things I'd like to try:

Also looking at that same run, I would suggest a couple of other changes:

  • do docker/login-action@v3 before checking if the image exists - I think if it's a private image then it will always say it does not exist if we do docker manifest inspect without logging in or otherwise might have issues
  • add if: ${{ steps.image_check.outputs.image_exists != 'true' }} for setup-qemu-action and setup-buildx-action since we only need to do that if we're actually going to build an image.

@kron-spar kron-spar requested a review from aelkiss March 8, 2024 20:47
Copy link
Copy Markdown
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

Makes sense to me as we discussed, although we should hold off on merging this until:

  1. we're happy with how this looks like when it gets called
  2. we're certain (esp. with the changes in tagging) whether it's backwards-compatible or needs a new version

Copy link
Copy Markdown
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

Moving the logic here looks good & with the test cases @Ronster2018 mentioned (might might be worth putting a link here) these should be ready to go.

@kron-spar
Copy link
Copy Markdown
Contributor Author

Test Cases:

- Default multi-platform support for amd64 and arm64 docker images.
- Updated versions for actions/checkout, actions/github-script, and docker/login-action
- Added img_tag field to separate github tags and container image tags.
- Added a core output for the id:latest_push. This way, if it's not the latest, we default to unstable while preserving the ability to add tags to the docker image as needed.
- Moved logic from the template build.yml file to this shared action.
@kron-spar kron-spar merged commit 8e3aed9 into main Mar 13, 2024
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.

2 participants