-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
chore(manifests): refactor manifests for kustomize5 compatibility. Part of #10053 #10087
chore(manifests): refactor manifests for kustomize5 compatibility. Part of #10053 #10087
Conversation
Hi @rawc0der. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
@rawc0der it is usually very helpful to join the kfp working group meeting to speed up PR review and discuss open issues and PRs |
/retest |
@zijianjoy this is definitely something that we want for Kubeflow 1.9 in the manifests WG. @rawc0der please fix the integration tests |
Great, then I'll find the time to do it this week! |
/retest |
Integration tests have been fixed. Some changes were required in the install scripts where older version of Kustomize More work is needed for replacing the remaining FYI @juliusvonkohout |
@rawc0der please join the KFP meeting on January 31 and put your PR on the agenda in the document in advance. This way we might get this merged soon. |
Sure, I will try to join the next KFP meeting to move this forward. |
Thank you for the contributions! Would you like to also update other places in the repo?
|
Sure, I can do that. Any other pointers? |
The testing mechanism is defined in this folder: https://github.com/GoogleCloudPlatform/oss-test-infra/tree/3f8905d98cdbb7005887113ae692b4024b83188f/prow/prowjobs/kubeflow/pipelines, you can take a look and see if anything needs to be updated. |
@rawc0der Did you have the chance to evaluate the tests? @zijianjoy Is there a way to run the postsubmit test in a local environment? |
I will try to spend some time today to wrap this up. |
@rawc0der Any updates? Is there something I can help you? |
@@ -23,7 +23,7 @@ TMP="$(mktemp -d)" | |||
|
|||
pushd "${TMP}" | |||
# Install Kustomize | |||
KUSTOMIZE_VERSION=3.10.0 | |||
KUSTOMIZE_VERSION=5.0.3 |
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.
hey @zijianjoy this is bumped already.
Hi @rimolive, I've run the test locally. No problems when building the templates. Here's the output.
kpt resources output``` $ kpt pkg tree ../ (⎈ |kind-k8s-1.26:default) .. ├── application │ ├── [application.yaml] ApplicationFrom my side think there are no more modifications ( |
Signed-off-by: Alin Spinu <[email protected]>
Signed-off-by: Alin Spinu <[email protected]>
Signed-off-by: Alin Spinu <[email protected]>
Signed-off-by: Alin Spinu <[email protected]>
Signed-off-by: Alin Spinu <[email protected]>
Signed-off-by: Alin Spinu <[email protected]>
ed299f4
to
11495a2
Compare
/retest |
Looks good to me. /lgtm |
/lgtm |
@zijianjoy Can you review/approve? |
@Tomcli Can you review/approve? |
Although this is probably safe, but let's wait until I cut a release first, so there's one less unknown (potential breaker). /hold |
@@ -44,7 +46,7 @@ if [ -z "$KFP_DEPLOY_RELEASE" ]; then | |||
KFP_MANIFEST_DIR=${DIR}/manifests | |||
|
|||
pushd ${KFP_MANIFEST_DIR}/cluster-scoped-resources | |||
kubectl apply -k . |
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.
Can users still use kubectl apply -k .
? Maybe it depends on the kubectl version, but if users only can use kustomize
, then we probably need to follow up with another PR to update the documents as well.
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.
/unhold
/lgtm
Undo LGTM. Waiting for @Tomcli to add LGTM once his comment is addressed. |
@rawc0der I want to confirm users can still use kubectl apply -k after this PR. This is not a blocker for this PR. I just want to make sure we are going to address the documentations after this PR is merged. |
The latest |
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
I opened another issue to track the doc changes. Otherwise it looks good to me.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun, Tomcli 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 |
The latest kubectl might soon have kustomize 5.2.1+ Maybe kubectl 1.30 or so. Nevertheless it will probably still work since this PR also worked with 5.0.3 before. But @Tomcli yes we should mention kustomize explicitly and just pipe it into kubectl apply as shown in kubeflow/manifests/readme.md. |
Description of your changes:
This PR is part of larger initiative #10053 to refactor manifests and addresses the problems of
kustomize5
compatibility.Warnings thrown before this PR:
Expected behaviour after refactoring the resources for
kustomize5
is to obtain healthy set of manifests without any warnings.Remaining Warnings (Work in progress):
Changes
This PR should fix the deprecated fields for the ml pipelines kustomization stack.
The deprecated fields in
kustomize5
to refactor:bases
->resources
patchesStrategicMerge
->patches
patchesJson6902
->patchces
vars
->replacements
After couple more commits I am expecting to have also refactored the remaining
vars
andpatchesJson6902
deprecations from the rest of the resources.Looking for feedback! (@annajung, @midhun1998)
Checklist: