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

fix most kustomize v5 warnings #2653

Conversation

AndersBennedsgaard
Copy link
Contributor

Which issue is resolved by this Pull Request:

Related to #2388

Description of your changes:

Fix most Kustomize v5 warnings related to for common/.
The warning about vars being deprecated is still present for oidc-client/oidc-authservice/base/kustomization.yaml as I weren't successful in fixing it without changes.
kustomize build generates no changes between old and new version.

Checklist:

  • Unit tests pass:
    Make sure you have installed kustomize == 5.0.3
    1. make generate-changed-only
    2. make test

@AndersBennedsgaard
Copy link
Contributor Author

AndersBennedsgaard commented Mar 13, 2024

@juliusvonkohout I've merged the PRs into one. As I mentioned in the PR description, I am not sure on how to fix vars being deprecated, so the oidc-authservice still produces a warning about that. And some tests are failing.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Mar 13, 2024

@AndersBennedsgaard we have commonLabels: in many components. I am not sure whether we should switch partially over to the other method (which i am also using myself). What is your argument for doing so? Maybe it is better to only do the kustomize 5 changes for now.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Mar 13, 2024

@diegolovison can you verify this on your local cluster?

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Mar 13, 2024

@AndersBennedsgaard patches/clusterrole-patch.yaml is not accepted by kustomize 5 i guess. Can you check how we can have multiple resources in the same file with kustomize 5 ? Maybe there is a new way to have a list of resources in a single file.

@AndersBennedsgaard
Copy link
Contributor Author

@AndersBennedsgaard we have commonLabels: in many components. I am not sure whether we should switch partially over to the other method (which i am also using myself). What is your argument for doing so? Maybe it is better to only do the kustomize 5 changes for now.

Because commonLabels are marked as deprecated. See kubernetes-sigs/kustomize#5436
commonLabels and labels with includeSelectors=true basically produce the same result, so kustomize edit fix changes it to labels.

@juliusvonkohout
Copy link
Member

If common labels is deprecated this is fine. I looked at multiple patches per file and found kubernetes-sigs/kustomize#5049

@AndersBennedsgaard
Copy link
Contributor Author

@juliusvonkohout should I split the patches into separate files instead then?

@juliusvonkohout
Copy link
Member

Maybe we have to upgrade to kustomize 5.3.0 kubernetes-sigs/kustomize#5194
kubernetes-sigs/kustomize#5059 (comment)

@juliusvonkohout
Copy link
Member

@juliusvonkohout should I split the patches into separate files instead then?

Try with kustomize 5.3.0 first please. If that works we might just upgrade kustomize

@AndersBennedsgaard
Copy link
Contributor Author

AndersBennedsgaard commented Mar 13, 2024

I am already using 5.3, which works as expected

Edit: it seems to have been fixed in 5.2.1: https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize%2Fv5.2.1

@juliusvonkohout
Copy link
Member

@rimolive based on the above comments i want to upgrade to kustomize 5.2.1+ and reflect this in the readme as well.

@diegolovison
Copy link
Contributor

@juliusvonkohout juliusvonkohout mentioned this pull request Mar 13, 2024
@juliusvonkohout
Copy link
Member

juliusvonkohout commented Mar 13, 2024

#2654 5.2.1 seems to be working with the manifests

@juliusvonkohout
Copy link
Member

@kimwnasptd what do you think?

@juliusvonkohout
Copy link
Member

If we are going to start support kustomize 5.x.x from this PR, I would like to ask to change:

* https://github.com/kubeflow/manifests/blob/master/tests/gh-actions/install_kustomize.sh

* https://github.com/kubeflow/manifests#prerequisites

* https://github.com/kubeflow/manifests/blob/master/.github/pull_request_template.md

Yes it is in #2654

@rimolive
Copy link
Member

@juliusvonkohout Are we good with merging this PR as we moved to 5.2.1?

@AndersBennedsgaard
Copy link
Contributor Author

AndersBennedsgaard commented Mar 15, 2024

@rimolive I think it would be better to hold this until #2654 has been merged, so unit tests can pass.
I also need to update

gateway. We will make this into a patch once we update kustomize to v4,

@juliusvonkohout
Copy link
Member

@AndersBennedsgaard feel free to rebase to master. I merged the other PR.

@AndersBennedsgaard AndersBennedsgaard force-pushed the fix/kustomize-v5-warnings branch from 7f64e1b to a3b2e8d Compare March 15, 2024 13:49
@google-oss-prow google-oss-prow bot added size/L and removed size/M labels Mar 15, 2024
@AndersBennedsgaard AndersBennedsgaard force-pushed the fix/kustomize-v5-warnings branch from a3b2e8d to 2ea2fa1 Compare March 15, 2024 13:53
Signed-off-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
@AndersBennedsgaard AndersBennedsgaard force-pushed the fix/kustomize-v5-warnings branch from 2ea2fa1 to 107856b Compare March 15, 2024 14:01
@google-oss-prow google-oss-prow bot added size/M and removed size/L labels Mar 15, 2024
@AndersBennedsgaard
Copy link
Contributor Author

I had a couple of git issues, but I think it is fine now. I have tried to add a patch for deleting the Knative-serving gateway as mentioned in the README. Do you think this is the right approach?

@juliusvonkohout
Copy link
Member

I might be able to review this on Monday.

@juliusvonkohout
Copy link
Member

Sorry for the delay, i probably have to postpone this to next week.

@juliusvonkohout
Copy link
Member

The diff seems to be zero

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndersBennedsgaard, juliusvonkohout

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 c4ba293 into kubeflow:master Apr 3, 2024
11 checks passed
@AndersBennedsgaard AndersBennedsgaard deleted the fix/kustomize-v5-warnings branch April 3, 2024 14:43
doncorsean pushed a commit to doncorsean/kubeflow-manifests that referenced this pull request Jul 18, 2024
Signed-off-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
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