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

Add multiarch #5

Merged
merged 4 commits into from
Jan 26, 2024
Merged

Add multiarch #5

merged 4 commits into from
Jan 26, 2024

Conversation

ArangoGutierrez
Copy link
Collaborator

Needed for devel on Mac MX

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
@@ -65,7 +67,8 @@ jobs:
CONTROLLER_GEN_VERSION: "v0.9.2"
CLIENT_GEN_VERSION: "v0.26.1"
run: |
docker build --build-arg GOLANG_VERSION=${GOLANG_VERSION} \
docker build --platform=linux/amd64,linux/arm64 \
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe that this will push the multi-arch image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I am looking how to

Copy link
Member

Choose a reason for hiding this comment

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

Note that for multi-arch images we use docker buildx and add the:

--output=type=image,push=$(PUSH_ON_BUILD)

flags to ensure that the multiarch image is pushed to the repo.

Copy link
Member

@elezar elezar Jan 26, 2024

Choose a reason for hiding this comment

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

For example, for the device plugin (note that the command below has push=false specified)

DOCKER_BUILDKIT=1 \
        docker buildx build --pull \
        --output=type=image,push=false \
        --platform=linux/amd64,linux/arm64 \
        --tag nvcr.io/nvidia/k8s-device-plugin:v0.14.3-ubuntu20.04 \
        --build-arg BASE_DIST="ubuntu20.04" \
        --build-arg CUDA_VERSION="12.3.0" \
        --build-arg GOLANG_VERSION="1.20.5" \
        --build-arg VERSION="v0.14.3" \
        --build-arg GIT_COMMIT="2bef25804caf5924f35a164158f097f954fe4c74-dirty" \
         \
        -f /Users/elezar/src/device-plugin/deployments/container/Dockerfile.ubuntu \
        /Users/elezar/src/device-plugin

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Is there any value in separating the build and push jobs if the only different should be:

 push: false

vs

 push: true

(and possibly the architecture?)

Or could you better describe the interaction between the two jobs?

.github/workflows/push.yaml Outdated Show resolved Hide resolved
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
@ArangoGutierrez
Copy link
Collaborator Author

Is there any value in separating the build and push jobs if the only different should be:

 push: false

vs

 push: true

(and possibly the architecture?)

Or could you better describe the interaction between the two jobs?

The just build, only build major X.X go version and on the default platform, not to waste time on building the big matrix.
Once the PR is in, the PUSH action, will build on arm and amd64 and on all the required go versions x.y.z

@elezar
Copy link
Member

elezar commented Jan 26, 2024

OK. That's fair. My concern is drift between the two jobs. Could we construct the matrix conditionally as per https://stackoverflow.com/questions/65384420/how-do-i-make-a-github-action-matrix-element-conditional (as well as push=true) and reuse the same step definition?

It seems as if using excludes (also in one of the solutions proposed in the linked page) could also do what we want:

strategy:
  matrix:
    fruits:
      - apple
      - banana
    exclude:
      - fruits: ${{ github.ref == 'refs/heads/main' && 'dummy' || 'apple' }}
      - fruits: ${{ (github.event_name != 'pull_request' || (github.event_name == 'pull_request' && contains( github.event.pull_request.labels.*.name, 'BUILD-BANANA'))) && 'dummy' || 'banana' }}

To be clear, I don't feel too strongly about this.

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

I still feel that we should reevaluate reusing the steps more to not have to update these in multiple places, but I don't see this as a blocker for moving forward in other projects.

Feel free to merge this to unblock and revisit the refactor in a follow-up.

@ArangoGutierrez ArangoGutierrez merged commit 3c1b8b4 into main Jan 26, 2024
2 checks passed
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