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

chore: update manifests to remove deprecation warnings #11675

Merged
merged 3 commits into from
Aug 27, 2023

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Aug 25, 2023

Motivation

  • kustomize was giving deprecation warnings:
    • patchesStratetgicMerge and patchesJson6902 have been deprecated and replaced by patches
    • commonLabels is deprecated and replaced by labels

As an example, see the "Install manifests" step of this recent CI run:

# Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.
# Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.
# Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.
# Warning: 'patchesJson6902' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.

Modifications

Verification

  • make manifests / make install runs without deprecation warnings.
  • Resulting generated manifests are identical, see below comment
  • E2E tests pass

Notes to Reviewers

Titled this as a build, but I'm not entirely sure what it should be EDIT: used chore instead

- `kustomize` was giving deprecation warnings:
  - `patchesStratetgicMerge` and `patchesJson6902` have been deprecated and replaced by `patches`
  - `commonLabels` is deprecated and replaced by `labels`

- run `kustomize edit fix` on all `kustomization.yaml`
  - then manually update to match the already used style/formatting in these files (e.g. reordering, indentation)

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Can you check if the generated manifests match the current ones?

@agilgur5
Copy link
Member Author

Oh like the release ones which are gitignore'd. Good point, let me double-check that to make sure.

@agilgur5
Copy link
Member Author

Also there are kustomization.yaml I missed in the tests as well. Will update those too in this PR

@agilgur5
Copy link
Member Author

Yep no diff in the generated manifests. The quick-start-*.yaml ones are still in the codebase too.

See check with no diff:

vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (fix-update-kustomize-patches) $ git checkout master
# [...]
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (master) $ make manifests
# [...]
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (master) $ cp manifests/install.yaml ../
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (master) $ cp manifests/namespace-install.yaml ../

vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (master) $ git checkout fix-update-kustomize-patches
# [...]
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (fix-update-kustomize-patches) $ make manifests
# [...]
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (fix-update-kustomize-patches) $ diff manifests/install.yaml ../install.yaml 
vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (fix-update-kustomize-patches) $ diff manifests/namespace-install.yaml ../namespace-install.yaml

@agilgur5 agilgur5 changed the title build: update manifests to remove deprecation warnings chore: update manifests to remove deprecation warnings Aug 25, 2023
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
Copy link
Member Author

Ah, converting tests is failing due to multiple manifests in one file (i.e. --- syntax), which is not supported yet: kubernetes-sigs/kustomize#5049.

Guess I'll have to partially revert the test changes and we'll just have deprecation warnings there until it is resolved upstream

- kustomize `patches` throws an error when there are two manifests in one file
- also every other mixin is a single manifest

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
Copy link
Member Author

Ah, converting tests is failing due to multiple manifests in one file (i.e. --- syntax), which is not supported yet: kubernetes-sigs/kustomize#5049.

There was only one file like this, so I just split that file instead, which should result in no errors and no deprecation warnings!

@terrytangyuan terrytangyuan merged commit 83fb5c3 into argoproj:master Aug 27, 2023
24 checks passed
@agilgur5 agilgur5 deleted the fix-update-kustomize-patches branch August 28, 2023 04:24
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.

2 participants