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

Push multi-arch images #80

Merged
merged 11 commits into from
Feb 16, 2023
Merged

Push multi-arch images #80

merged 11 commits into from
Feb 16, 2023

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Jan 16, 2023

Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
@ddelange ddelange changed the title Fix multi-arch builds Push multi-arch images Jan 16, 2023
already tested for pull_request event: https://github.com/kserve/modelmesh/blob/v0.10.0/.github/workflows/pull-request-validation.yml

Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
@ckadner ckadner requested review from ckadner and removed request for animeshsingh January 20, 2023 23:04
@ckadner
Copy link
Member

ckadner commented Jan 20, 2023

@ddelange -- the .github/workflows/pull-request-validation.yml workflow needs to be changed as well so that the multi-arch build gets tested on PRs including this one :-)

It might be better to consolidate the two workflow files into one file and work with conditions to not push images to DockerHub for PR validation (only on push to main or pushing tags)

@ddelange
Copy link
Contributor Author

SGTM, I'll take some time early next week to:

  • add pull_request trigger
  • add conditionals so there's no pushing on PR
  • remove the other workflow yaml

would you like me to update the other PRs in the same fashion?

@ckadner
Copy link
Member

ckadner commented Jan 21, 2023

would you like me to update the other PRs in the same fashion?

Thanks @ddelange -- yup, we should consolidate the workflow files on all the modelmesh repos:

Unfortunately they all have somewhat different setups currently with respect to the Dockerfile (Dockerfile.develop), and the related docker build instructions in the README files, the build commands in the Makefile and sometimes in build scripts, on top of the GitHub workflows. So there is a bit of complexity to figure out that might take up more time than you had bargained for :-)

Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
@ddelange
Copy link
Contributor Author

@ckadner Ci is green:) maven didn't like s390x arch so I removed it

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Looks good. Just two minor questions :-)

.github/workflows/build-and-push.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-push.yml Outdated Show resolved Hide resolved
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
@ddelange ddelange removed the request for review from njhill January 31, 2023 09:22
@ddelange ddelange requested a review from ckadner January 31, 2023 09:22
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Great. Thanks @ddelange -- let's remove the the commented out code pieces and the extra IMAGE_ID/IMAGE_NAME changes

.github/workflows/build-and-push.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-push.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-push.yml Outdated Show resolved Hide resolved
Signed-off-by: ddelange <14880945+ddelange@users.noreply.github.com>
@ddelange ddelange requested a review from ckadner February 1, 2023 07:30
Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks @ddelange

Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks a lot for all your work on this @ddelange @ckadner!

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckadner, ddelange, njhill

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@njhill
Copy link
Member

njhill commented Feb 16, 2023

/lgtm

@ckadner ckadner merged commit e6197b4 into kserve:main Feb 16, 2023
@ckadner ckadner mentioned this pull request May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants