- 
                Notifications
    You must be signed in to change notification settings 
- Fork 67
feat: Migrate kubeflow/kubeflow jwa_docker_publish.yaml #626
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
feat: Migrate kubeflow/kubeflow jwa_docker_publish.yaml #626
Conversation
42977f5    to
    f49698e      
    Compare
  
    | /hold I want to make sure: 
 | 
f49698e    to
    43c1c6b      
    Compare
  
    1e36649    to
    91bc27c      
    Compare
  
    a08ef33    to
    5c0ce13      
    Compare
  
    | - name: Setup QEMU | ||
| uses: docker/setup-qemu-action@v3 | ||
| with: | ||
| platforms: amd64,ppc64le,arm64 | 
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.
this is interesting but not included in the original https://github.com/kubeflow/kubeflow/blob/master/.github/workflows/jwa_docker_publish.yaml
can you comment on the benefits of adding this setting?  and furthermore - could we use {{env.ARCH}} here to not have a "future sync issue" with this being separately hard-coded?
If we cannot use {{env.ARCH} outright - I'd probably suggest removing this.. but I'm still interested in understanding what prompted the inclusion!
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.
Ok, so to be honest, I think this is leftover from some previous issues we encountered with the ARM platform. The main purpose of this step is just to specify which platforms to install, whereas the default behavior would be to set up QEMU for all platforms. Unfortunately, we can’t directly use {{env.ARCH}} here because it includes the linux/ prefix. So overall, I think the cleanest approach would be to remove this step entirely.
| /ok-to-test | 
… to kubeflow/notebooks - notebooks-v1 branch Signed-off-by: Yehudit Kerido <[email protected]>
5c0ce13    to
    65a3f61      
    Compare
  
    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.
/lgtm
Thanks @yehudit1987 - good to finally be at a place to get this merged. appreciate your work!
Verified on fork:
- https://github.com/andyatmiami/kubeflow-notebooks/actions/runs/18958843845/job/54141699277
- note: small commit to point to my repo
 
Image available:
| /unhold | 
| Thanks for the yet another great PR @yehudit1987! /approve | 
| [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  | 
Migrate Jupyter Web App Docker publish workflow from kubeflow/kubeflow
to kubeflow/notebooks with multi-platform build improvements.
Key Changes
Migration
Successful build: https://github.com/yehudit1987/notebooks/actions/runs/18833587264/job/53729515972