-
Notifications
You must be signed in to change notification settings - Fork 114
Added multiarch support by docker buildx #336
Conversation
FYI - @nickboldt - Follow up PR as per comments here - https://issues.redhat.com/browse/CRW-1475?focusedCommentId=15721358&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15721358 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes are required, please see comments inline. Also, please test to make sure that the regression from the last attempt doesn't happen this time.
@ericwill Can you please review this PR, I have changed Accordingly Below change requests are updated
|
@ericwill added Platform file in arbitrary-users-patch and arbitrary-users-patch/happy-path location, accordingly modified the script. |
@bivasda1 please rebase the PR and let's merge it next week. There is a |
@mkuznyetsov FYI ^ @bivasda1 if rebased and ready by EOD Tuesday, we can include in Wednesday's release. If not... will slip to the week after. Note too that we've been having issues with qemu emulated s390x buildx builds, so you might want to only include ppc64le in this PR as the s390x build might make it harder to validate. |
@nickboldt I will remove s390x form Platform files and will rebase as quickly as possible. |
@nickboldt @ericwill I have rebased. |
.github/workflows/release.yml
Outdated
context: . | ||
file: ./build/dockerfiles/Dockerfile | ||
platforms: ${{ steps.prep.outputs.platforms }} | ||
tags: quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ steps.prep.outputs.version }},quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ steps.prep.outputs.short_sha1 }},quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ github.event.inputs.version }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're pushing the same image w/ 3 tags here:
-
quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ steps.prep.outputs.version }},
-
quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ github.event.inputs.version }}
-
quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ steps.prep.outputs.short_sha1 }},
should it be only two pushes for prep.outputs.version and short_sha1? Not sure why we would want the submitted version 7.y.z AND version from VERSION file AND the short sha from the tip of the checked out branch.
@mkuznyetsov WDYT?
Also there's an extra space in ${{ github.event.inputs.version }}
which might cause error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be only two pushes for prep.outputs.version and short_sha1?
Yes
prep.outputs.version-->7.27.0-SNAPSHOT
event.inputs.version-->7.27.0
I was trying to release proper version, so that event.inputs.version
I have added. Please let me know should I remove event.inputs.version
?
I will remove spaces from ${{ github.event.inputs.version }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't release -SNAPSHOT versions to quay.
https://quay.io/repository/eclipse/che-devfile-registry?tab=tags
Just nightly + sha tag and release (7.yy.z) + sha tag.
.github/workflows/release.yml
Outdated
context: . | ||
file: ./build/dockerfiles/Dockerfile | ||
platforms: ${{ steps.prep.outputs.platforms }} | ||
tags: quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ steps.prep.outputs.version }},quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ steps.prep.outputs.short_sha1 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericwill @nickboldt I have removed quay.io/eclipse/${{ steps.prep.outputs.image }}:${{ github.event.inputs.version }}
and rebased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outputs.version-->7.27.0-SNAPSHOT
That's the wrong version to release. We would want 7.27.0, not the -SNAPSHOT version. Releases are for releases, nightly is for snapshots. :D
run: | | ||
SHORT_SHA1=$(git rev-parse --short HEAD) | ||
echo ::set-output name=short_sha1::${SHORT_SHA1} | ||
VERSION=$(head -n 1 VERSION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this value != inputs.version
we should use inputs.version.
In fact I'm not sure why we need BOTH a computed value (7.27.0-SNAPSHOT) and an input value (7.27.0).
I would expect that if the input is missing, we compute it... but I believe part of the release process is to SET the value in the VERSION file, so there's no point reading the file before we set the value. :)
Instead I'd just take the value from the input as it should be the same as the VERSION file, once the release is complete.
Correct @mkuznyetsov ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickboldt do you expect ci-multiarch workflows should be formatted https://github.com/eclipse/che-dashboard/blob/master/.github/workflows/ci-multiarch.yaml#L91-L115 referenced to eclipse-che/che-dashboard#119 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickboldt as of now, make-release.sh is called first from another workflow, which makes changes to VERSION file, and then this workflow does the rest. Here it reads the VERSION file, in case it was triggered by the push to the release branch, which has no inputs
Because it was slightly confusing, I made a PR (about which I've forgot 😃 )
https://github.com/eclipse/che-devfile-registry/pull/338/files
it makes make-release.sh
to be called from this workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So which approach are we taking? Right now as it stands, the 7.27.0-SNAPSHOT
problem won't happen because the workflow is triggered after the file is changed. But we can have this PR wait until https://github.com/eclipse/che-devfile-registry/pull/338/files is merged and then adjust the approach. What do you guys think? cc @nickboldt @mkuznyetsov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR 338 is ready to merge, after fixing all the shellcheck complaints. https://github.com/eclipse/che-devfile-registry/pull/338/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've merged it, @bivasda1 please adjust the PR to make sure -SNAPSHOT
isn't pushed as part of the tag
Since I don't own this project, I'll let the TL comment... but my $0.02 is that it seems like from a project management perspective it might be better/more efficient to have:
|
.github/workflows/release.yml
Outdated
required: true | ||
default: '7.y.z' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest having an empty default value, so we wouldn't have accidental workflow runs with 7.y.z
taken as a default parameter.
@ericwill Could you please share your thoughts @nickboldt suggestions |
@nickboldt Could you please review this PR,I think Versioning issue is resolved now. |
0548984
to
953c109
Compare
Could u please review the PR I have deleted |
please rebase, you're out of date again :( |
Signed-off-by: Bivas Das <bivasda1@in.ibm.com>
@nickboldt Rebased and Added buildx support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give it a shot
seems legit for x and power arches but this won't work for s390x and will need to be refactored in future. +1 for pushing this for now, with the caveat that we'll need something like eclipse-che/che-dashboard#177 to support pushing each per-arch to quay independently, then combining them into a single multi-arch digest/manifest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, but won't work for s390x.
Just commenting...here is a bash script very similar to what I do for building multi-arch images. As @nickboldt mentioned from eclipse-che/che-dashboard#177 that there is a risk of building for
FYI |
@bivasda1 so the build was successful, but please fix/update the following in a new PR:
https://quay.io/repository/eclipse/che-devfile-registry?tab=tags |
There, I fixed it. :) https://quay.io/repository/eclipse/che-devfile-registry?tab=tags&tag=nightly Also deleted the 7.28.0-SNAPSHOT tag |
But also... the image is not multiarch :(
-- https://github.com/eclipse/che-devfile-registry/runs/2057571221?check_suite_focus=true and if I skopeo inspect I only get amd64:
Contrast with:
|
@nickboldt I am curious how this will get resolved, because this is similar to the issue I faced when trying to build registry images for |
well, there's the approach in https://github.com/eclipse/che-dashboard/blob/master/.github/workflows/ci-multiarch.yaml ... |
with: | ||
context: . | ||
file: ./build/dockerfiles/Dockerfile | ||
platforms: ${{ steps.package.outputs.content }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickboldt Image is still single arch because I guess slight change is required here. Instead of platforms: ${{ steps.package.outputs.content }}
, it needs to be platforms: ${{ steps.prep.outputs.platforms }}
as defined on line 37.
@nickboldt @ericwill
Even we can enable multiarch for these repos.
|
Yes we need to exclude stacks that aren't available on multi-arch. For example if rust is x86 only, then it should be excluded from builds on other arches. |
For arch specific devfile exclusion filtering, see what we're dong downstream in CRW:
--> moved to eclipse-che/che#19239 |
@nickboldt Thanks for your details :) |
Partial revert of #336 Related to eclipse-che/che#19486 Signed-off-by: Eric Williams <ericwill@redhat.com>
Partial revert of #336 Related to eclipse-che/che#19486 Signed-off-by: Eric Williams <ericwill@redhat.com>
Signed-off-by: Bivas Das bivasda1@in.ibm.com
What does this PR do?
Enables docker buildx support in GitHub Actions job to build and publish multiarch docker images. All the workflows (nightly build, PR check, and release) are updated to have multiarch support.
What issues does this PR fix or reference?
This refers to #321
Reviewers
@ericwill Could you please review this PR