Skip to content

Centralise Docker setup actions (qemu + buildx) #1036

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

Merged
merged 4 commits into from
Aug 7, 2025
Merged

Conversation

JackPGreen
Copy link
Collaborator

@JackPGreen JackPGreen commented Jul 29, 2025

The docker/setup-buildx-action@v3 with version: v0.5.1 is duplicated in 7 actions. The version used for the build system should be specified only once - extracted.

This action could have been externalised to docker-actions but seemed overengineered, instead access directly on master - but until merged, the builds will fail. Example execution when hardcoded to execute on this branch.

Note - this version is old which is what prompted centralising to make it easier to explore upgrading in future.

The `docker/setup-buildx-action@v3` with `version: v0.5.1` is duplicated in 7 actions. The version used for the build system should be specified only once - extracted.

Note - this version is [old](https://github.com/docker/buildx/releases/tag/v0.5.1) which is what prompted centralising to make it easier to explore upgrading in future.
@JackPGreen JackPGreen self-assigned this Jul 29, 2025
@JackPGreen JackPGreen marked this pull request as ready for review July 29, 2025 21:32
@JackPGreen JackPGreen requested a review from a team as a code owner July 29, 2025 21:32
@JackPGreen JackPGreen enabled auto-merge (squash) July 29, 2025 21:32
@nishaatr
Copy link
Contributor

Good change but I think this should ideally go in docker-actions
I believe the end goal is to have all actions there and then have minimal business logic here
It will cause less confusion IMO

@JackPGreen
Copy link
Collaborator Author

Good change but I think this should ideally go in docker-actions I believe the end goal is to have all actions there and then have minimal business logic here It will cause less confusion IMO

I thought we thought we had to architect it like that to workaround GitHub issues, not that we wanted to - were a long way off having only Dockerfiles in this repo. I'd be open to @ldziedziul opinion though.

@nishaatr
Copy link
Contributor

Good change but I think this should ideally go in docker-actions I believe the end goal is to have all actions there and then have minimal business logic here It will cause less confusion IMO

I thought we thought we had to architect it like that to workaround GitHub issues, not that we wanted to - were a long way off having only Dockerfiles in this repo. I'd be open to @ldziedziul opinion though.

Yes very true but @ldziedziul also made a compelling argument that having code in docker-actions also means we have physically one copy of actions as opposed to in hazelcast-docker where it gets copied over to all branches unnecessarily (and causing possible confusion). lets see what @ldziedziul says and then I will review

@ldziedziul
Copy link
Contributor

ldziedziul commented Aug 5, 2025

I thought we thought we had to architect it like that to workaround GitHub issues, not that we wanted to - were a long way off having only Dockerfiles in this repo.

Our main goal is to avoid duplicating workflow logic. However, using bash scripts inside reusable workflows is problematic, as they're executed in the context of the current branch. That means we would need to explicitly check out additional scripts into a separate directory to make it work.

A better alternative is to wrap that logic in GitHub Actions. Actions are first-class citizens in GitHub workflows, and this brings several benefits:

  • Reduced complexity in the workflow files themselves.

  • Improved testability, since many scripts (and logic around it, I/O, validation etc) weren’t properly tested until they were used in a workflow.

  • A separation of concerns between workflows (orchestration) and actions (implementation), which defines clean APIs and boundaries.

  • (as @nishaatr pointed out), a single physical copy of each action, unlike in hazelcast-docker where the logic ends up duplicated across all branches which can lead to confusion and inconsistency.

  • (ADDED): Most of these benefits one can achieve using actions in master branch, but then we bloat hazelcast-docker PRs with test workflows needed only when github actions are changed, which adds noise to otherwise unrelated changes.

  • (ADDED): No need to re-tag released version when we fix logic in the workflows needed for automatic rebuilds

Copy link
Contributor

@ldziedziul ldziedziul left a comment

Choose a reason for hiding this comment

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

We can extract it, it's a good direction, but we should consider moving this into docker-actions

@JackPGreen JackPGreen disabled auto-merge August 5, 2025 13:51
@JackPGreen JackPGreen merged commit 4b11e5b into master Aug 7, 2025
10 of 16 checks passed
@JackPGreen JackPGreen deleted the centralise-buildx branch August 7, 2025 22:49
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.

3 participants