fix most kustomize v5 warnings

AndersBennedsgaard commented 3 months ago

Which issue is resolved by this Pull Request:

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@diegolovison can you verify this on your local cluster?

juliusvonkohout commented 3 months ago

@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 commented 3 months ago

@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 https://github.com/kubernetes-sigs/kustomize/issues/5436 commonLabels and labels with includeSelectors=true basically produce the same result, so kustomize edit fix changes it to labels.

juliusvonkohout commented 3 months ago

If common labels is deprecated this is fine. I looked at multiple patches per file and found https://github.com/kubernetes-sigs/kustomize/issues/5049

AndersBennedsgaard commented 3 months ago

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

juliusvonkohout commented 3 months ago

Maybe we have to upgrade to kustomize 5.3.0 https://github.com/kubernetes-sigs/kustomize/pull/5194 https://github.com/kubernetes-sigs/kustomize/pull/5059#issuecomment-1775515074

juliusvonkohout commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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

diegolovison commented 3 months ago

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

juliusvonkohout commented 3 months ago

https://github.com/kubeflow/manifests/pull/2654 5.2.1 seems to be working with the manifests

juliusvonkohout commented 3 months ago

@kimwnasptd what do you think?

juliusvonkohout commented 3 months ago

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 https://github.com/kubeflow/manifests/pull/2654

rimolive commented 3 months ago

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

AndersBennedsgaard commented 3 months ago

@rimolive I think it would be better to hold this until https://github.com/kubeflow/manifests/pull/2654 has been merged, so unit tests can pass. I also need to update https://github.com/kubeflow/manifests/blob/68652a1b0262378e823f58b1d07bfb8d7a8be006/common/knative/README.md?plain=1#L40

juliusvonkohout commented 3 months ago

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

AndersBennedsgaard commented 3 months ago

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 commented 3 months ago

I might be able to review this on Monday.

juliusvonkohout commented 3 months ago

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

juliusvonkohout commented 3 months ago

The diff seems to be zero

/lgtm /approve

google-oss-prow[bot] commented 3 months ago

[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: - ~~[OWNERS](https://github.com/kubeflow/manifests/blob/master/OWNERS)~~ [juliusvonkohout] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

kubeflow / manifests

fix most kustomize v5 warnings #2653