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

Adding changes to build JWA on pull_request #6992

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

amitmukati-2604
Copy link
Contributor

No description provided.

@amitmukati-2604
Copy link
Contributor Author

@kimwnasptd
Copy link
Member

Thanks for the PR @amitmukati-2604!

I'd like to keep the *_publish.yaml files for running only when the PR is merged. Could you modify instead the *_integration_test.yaml workflow, which are already building the images, and build for ppc64le there?

You can take a look at #6961 where we did just that for the CentralDashboard

@lehrig
Copy link
Contributor

lehrig commented Feb 24, 2023

@kimwnasptd, I think we should have a general discussion and agreement on how to proceed with (a) builds on pull request and (b) integration testing; it is really a cross-component concern affecting multiple PRs.

The issue is this:

  • we already enabled multi-arch builds when a PR is merged (*_publish.yaml will then be relevant)
  • we want to run multi-arch builds on pull requests (you propose integrating via *_integration_test.yaml)
  • the testing part of *_integration_test.yaml fails on non-x86 archs because it currently requires native hardware

Proposal:

  1. Let us not run tests for other archs than x86. thus far we never experienced differences in x86 vs. ppc64le, so the value we get from testing would be small (and hey, testing is not validation). Additionally, I see the distribution providers vor non-x86 archs in the responsibility of testing thoroughly (and we from IBM and our partners will do so for ppc64le).
  2. For builds on pull requests, we either (a) just trigger the build inside *_integration_test.yaml but let the test only run for the x86 piece or (b) separate workflow login by first having a workflow for building; then having a second workflow for testing (x86 only).

We have some projects where we provision dedicated native hardware; or where we enable tests via QEMU. I currently don't believe this is needed here.

Thoughts?

@kimwnasptd
Copy link
Member

Thanks for taking the time to expose this so that we are in sync @lehrig!

Yes, I agree that for now it's more than enough to focus on ensuring the ppc64le images can be built and not care about testing them as well. This wasn't clear from my message above.

I just want to keep the *_publish.yaml actions for running only once a PR is merged. We have some bits of logic that took some effort to finalize, like when to tag latest, commit hashes etc, that I'd like to keep them as they are and not bloat these actions more. cc @apo-ger

So let's go with a more lightweight version of your second proposal: Let's just extend the *_integration_test.yaml files to also build for ppc64le and keep testing the x86 images in the tests. If in the future we see issues and get feedback that some features don't work on ppc64le images then let's discuss again on what to better test, based on the cases we'll have.

Note, this is also what we did for the CentralDashboard in #6961

So I think we are in sync, but please tell me if you think I'm missing something

@lehrig
Copy link
Contributor

lehrig commented Feb 28, 2023

@kimwnasptd sounds all good to me and the team; we're proceeding accordingly - thanks!

@amitmukati-2604
Copy link
Contributor Author

@kimwnasptd
Copy link
Member

Looks good @amitmukati-2604, thanks!

/lgtm
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

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

@google-oss-prow google-oss-prow bot merged commit a6e6e71 into kubeflow:master Mar 21, 2023
Adembc pushed a commit to Adembc/notebooks that referenced this pull request Jun 22, 2024
* Adding changes to build JWA on pull_request

* Adding changes to build JWA on pull_request
Adembc pushed a commit to Adembc/notebooks that referenced this pull request Jun 22, 2024
* Adding changes to build JWA on pull_request

* Adding changes to build JWA on pull_request
Adembc pushed a commit to Adembc/notebooks that referenced this pull request Jun 22, 2024
* Adding changes to build JWA on pull_request

* Adding changes to build JWA on pull_request
Adembc pushed a commit to Adembc/notebooks that referenced this pull request Jun 23, 2024
* Adding changes to build JWA on pull_request

* Adding changes to build JWA on pull_request
Adembc pushed a commit to Adembc/notebooks that referenced this pull request Jun 23, 2024
* Adding changes to build JWA on pull_request

* Adding changes to build JWA on pull_request
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.

None yet

3 participants