Skip to content

Conversation

@ishavkon
Copy link

@ishavkon ishavkon commented Sep 18, 2025

resolves #591

migrate the files below to kubeflow/notebooks-v1 branch:

  • tb_controller_multi_arch_test.yaml
  • tb_controller_integration_test.yaml

Copy link
Contributor

@andyatmiami andyatmiami left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions - changes I am requesting are hopefully obvious/straightforward...

also please be aware - depending on which notebooks-v1 workflow PR is ready to merge first - we may also need a rebase as some of the testing/gh-actions files are also being created in additional PRs being raised.

@google-oss-prow google-oss-prow bot added area/controller area - related to controller components area/v1 area - version - kubeflow notebooks v1 labels Sep 22, 2025
@andyatmiami
Copy link
Contributor

/ok-to-test

Copy link
Contributor

@andyatmiami andyatmiami left a comment

Choose a reason for hiding this comment

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

Thanks for your continued engagement.. getting closer to having something merge-able.. but still need to do a little more refining.

Please reach out to me if you need/want more details about how you can test this on your fork to gain confidence in its functionality on the upstream repo.

Copy link
Contributor

@andyatmiami andyatmiami left a comment

Choose a reason for hiding this comment

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

Thanks again for your continued attention to this PR... just a couple more things to clear up..

  • please ensure the DCO checks on your PR are passing.. you can probably squash some/all of your commits if you want to avoid having to make sure each one is signed
  • fix up the indenting that was introduced (i'm guessing unintentionally) to kind-1-25.yaml

Comment on lines 5 to 7
- |-
[plugins."io.containerd.grpc.v1.cri".registry.mirrors."$REGISTRY_NAME:$REGISTRY_PORT"]
endpoint = ["http://$REGISTRY_NAME:$REGISTRY_PORT"]
Copy link
Contributor

@andyatmiami andyatmiami Sep 29, 2025

Choose a reason for hiding this comment

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

Looks like you introduced an indentation here that was not present in the source originally:

Preserving original formatting will avoid a merge conflict if/as some other PR that includes this file gets merged first.

Suggested change
- |-
[plugins."io.containerd.grpc.v1.cri".registry.mirrors."$REGISTRY_NAME:$REGISTRY_PORT"]
endpoint = ["http://$REGISTRY_NAME:$REGISTRY_PORT"]
- |-
[plugins."io.containerd.grpc.v1.cri".registry.mirrors."$REGISTRY_NAME:$REGISTRY_PORT"]
endpoint = ["http://$REGISTRY_NAME:$REGISTRY_PORT"]

Comment on lines 20 to 24
nodes:
- role: control-plane
image: kindest/node:v1.25.3@sha256:f52781bc0d7a19fb6c405c2af83abfeb311f130707a0e219175677e366cc45d1
- role: worker
image: kindest/node:v1.25.3@sha256:f52781bc0d7a19fb6c405c2af83abfeb311f130707a0e219175677e366cc45d1 No newline at end of file
Copy link
Contributor

@andyatmiami andyatmiami Sep 29, 2025

Choose a reason for hiding this comment

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

Looks like you introduced an indentation here that was not present in the source originally:

Preserving original formatting will avoid a merge conflict if/as some other PR that includes this file gets merged first.

Suggested change
nodes:
- role: control-plane
image: kindest/node:v1.25.3@sha256:f52781bc0d7a19fb6c405c2af83abfeb311f130707a0e219175677e366cc45d1
- role: worker
image: kindest/node:v1.25.3@sha256:f52781bc0d7a19fb6c405c2af83abfeb311f130707a0e219175677e366cc45d1
nodes:
- role: control-plane
image: kindest/node:v1.25.3@sha256:f52781bc0d7a19fb6c405c2af83abfeb311f130707a0e219175677e366cc45d1
- role: worker
image: kindest/node:v1.25.3@sha256:f52781bc0d7a19fb6c405c2af83abfeb311f130707a0e219175677e366cc45d1

Copy link
Contributor

@andyatmiami andyatmiami 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 for all this work @ishavkon - appreciate your responsiveness in working through my comments.

I'm comfortable with the changes and have verified the workflows behave as expected.

Tested on my fork:

image

@google-oss-prow google-oss-prow bot added the lgtm label Oct 20, 2025
 migrate the files below to kubeflow/notebooks-v1 branch
 - tb_controller_multi_arch_test.yaml
 - tb_controller_integration_test.yaml

Signed-off-by: Ilya Shavkonov <ishavkon@redhat.com>
@andyatmiami
Copy link
Contributor

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 21, 2025
@kimwnasptd
Copy link
Member

Thank you for the great work @ishavkon and the thorough reviews @andyatmiami!

/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 465cad2 into kubeflow:notebooks-v1 Oct 21, 2025
9 checks passed
@github-project-automation github-project-automation bot moved this from Needs Triage to Done in Kubeflow Notebooks Oct 21, 2025
@ishavkon ishavkon deleted the task591 branch October 30, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved area/ci area - related to ci area/controller area - related to controller components area/v1 area - version - kubeflow notebooks v1 lgtm ok-to-test size/L

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants