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 Kustomization Warnings #4859

Merged

Conversation

nawazkh
Copy link
Member

@nawazkh nawazkh commented May 16, 2024

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR is intended to fix Kustomization warnings seen on running make generate.
The warnings I see are

  • # Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.
  • Flag --reorder has been deprecated, use the new 'sortOptions' field in kustomization.yaml instead.

Which issue(s) this PR fixes:

Fixes #4752

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

NONE

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 16, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 16, 2024
Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.01%. Comparing base (31dd10c) to head (f0f1853).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4859   +/-   ##
=======================================
  Coverage   62.01%   62.01%           
=======================================
  Files         201      201           
  Lines       16862    16862           
=======================================
  Hits        10457    10457           
  Misses       5622     5622           
  Partials      783      783           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member Author

@nawazkh nawazkh left a comment

Choose a reason for hiding this comment

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

Self notes.

config/capz/kustomization.yaml Outdated Show resolved Hide resolved
config/certmanager/kustomization.yaml Outdated Show resolved Hide resolved
config/crd/kustomization.yaml Outdated Show resolved Hide resolved
config/default/kustomization.yaml Outdated Show resolved Hide resolved
config/manager/kustomization.yaml Show resolved Hide resolved
config/webhook/kustomization.yaml Outdated Show resolved Hide resolved
templates/test/dev/custom-builds/kustomization.yaml Outdated Show resolved Hide resolved
@nawazkh nawazkh force-pushed the fix_kustomization_warnings branch 2 times, most recently from 16b457d to de1b168 Compare May 16, 2024 22:28
hack/update_kustomize.sh Outdated Show resolved Hide resolved
@nawazkh nawazkh force-pushed the fix_kustomization_warnings branch from de1b168 to 28c3efb Compare May 17, 2024 00:07
@nawazkh nawazkh marked this pull request as ready for review May 17, 2024 00:07
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2024
@nawazkh nawazkh added this to the v1.16 milestone May 17, 2024
@mboersma mboersma mentioned this pull request May 17, 2024
4 tasks
@nawazkh nawazkh force-pushed the fix_kustomization_warnings branch from 28c3efb to 2570c4d Compare May 17, 2024 17:12
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

I think the goal of this PR should be just to change any kustomization.yaml files (and the Makefile, etc.) but not have any of the generated templates change. At least that's what I was attempting in #4753. You may have to add

sortOptions:
  order: fifo

to ensure things stay stable.

config/capz/kustomization.yaml Outdated Show resolved Hide resolved
config/capz/kustomization.yaml Outdated Show resolved Hide resolved
@nawazkh nawazkh force-pushed the fix_kustomization_warnings branch 2 times, most recently from 0d199ff to 390d257 Compare May 17, 2024 19:45
@nawazkh
Copy link
Member Author

nawazkh commented May 17, 2024

I think the goal of this PR should be just to change any kustomization.yaml files (and the Makefile, etc.) but not have any of the generated templates change. At least that's what I was attempting in #4753. You may have to add

sortOptions:
  order: fifo

to ensure things stay stable.

order: fifo did the trick. Thank you for suggesting it.
The changes are now local to "*kustomization.yaml".
For my better understanding, apart from lowering the churn created with generated templates, is there any other reason to choose "fifo" over default "legacy" sortOptions?

@nawazkh
Copy link
Member Author

nawazkh commented May 17, 2024

/test pull-cluster-api-provider-azure-e2e-aks
pull-cluster-api-provider-azure-e2e-aks appears to be a time out error.

@nawazkh
Copy link
Member Author

nawazkh commented May 17, 2024

/test pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts
pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts also appears to be a timeout error.

@mboersma
Copy link
Contributor

is there any other reason to choose "fifo" over default "legacy" sortOptions?

AFAICT sortOptions: fifo keeps the template YAML in the order it was defined, same as --reorder none did. I assume we did this to make it easier to compare the sources with the generated templates, or to avoid unneeded diffs.

@nawazkh nawazkh force-pushed the fix_kustomization_warnings branch 2 times, most recently from 20e44c1 to 596da46 Compare May 17, 2024 23:15
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2375193756d1d0846b09f8102138dca6528e8d13

@willie-yao
Copy link
Contributor

/retest

Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

Overall looks good! Will run this locally to verify and I'll approve it then

/lgtm

done

for name in $(find "${dev_dir}"* -maxdepth 0 -type d -print0 | xargs -0 -I {} basename {} | grep -v patches); do
${KUSTOMIZE} build --load-restrictor LoadRestrictionsNone --reorder none "${dev_dir}${name}" > "${dev_dir}cluster-template-${name}.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own understanding: why is --reorder none removed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

kustomize v5.4.1 complained on using reorder flag. It said Flag --reorder has been deprecated, use the new 'sortOptions' field in kustomization.yaml instead

Copy link
Member Author

Choose a reason for hiding this comment

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

So we removed --reorder flag and are using sortOptions instead in the kustomization files

@nawazkh
Copy link
Member Author

nawazkh commented May 20, 2024

/hold

pull-cluster-api-provider-azure-verify seems to be generating a diff for the filetemplates/test/dev/cluster-template-custom-builds-machine-pool.yaml which should not be happening. I will investigate on why this is happening.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2024
- config folder
- templates/addons
- templates/azure-cluster-identity
- templates/flavors
- templates/test/ci
- templates/test/dev
- hack/observabilty
- test/e2e/data

remove --reorder none flag from the kustomize command and add sortOption:legacy to below folder
- flavors
- test/ci
- test/dev
@nawazkh nawazkh force-pushed the fix_kustomization_warnings branch from 596da46 to f0f1853 Compare May 20, 2024 17:52
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2024
Comment on lines +7 to +10
- path: ../patches/control-plane-custom-builds.yaml
- path: patches/machine-pool-deployment-pr-version-windows.yaml
- path: patches/custom-builds.yaml
- path: ../../../test/ci/prow-ci-version/patches/oot-credential-provider-kcp.yaml
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the sortOrder is set to fifo, a change in the order of listing these patch files generates a different cluster-template-custom-builds-machine-pool.yaml

@nawazkh
Copy link
Member Author

nawazkh commented May 20, 2024

/hold

pull-cluster-api-provider-azure-verify seems to be generating a diff for the filetemplates/test/dev/cluster-template-custom-builds-machine-pool.yaml which should not be happening. I will investigate on why this is happening.

/unhold
This should now be resolved since I have restored the original order for the patch files in templates/test/dev/custom-builds-machine-pool/kustomization.yaml

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2024
@nawazkh
Copy link
Member Author

nawazkh commented May 20, 2024

/hold
pull-cluster-api-provider-azure-verify seems to be generating a diff for the filetemplates/test/dev/cluster-template-custom-builds-machine-pool.yaml which should not be happening. I will investigate on why this is happening.

/unhold This should now be resolved since I have restored the original order for the patch files in templates/test/dev/custom-builds-machine-pool/kustomization.yaml

And I do not believe we need to worry about restoring the order of patch files in other kustomization.yaml files since make generate no longer generates a diff for the generated flavors.

@nawazkh
Copy link
Member Author

nawazkh commented May 20, 2024

/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts

Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ddbf42e15e90aac3e2e8eee41be84bc860ce055f

Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: willie-yao

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2024
@k8s-ci-robot k8s-ci-robot merged commit 76240e7 into kubernetes-sigs:main May 21, 2024
27 checks passed
@nawazkh nawazkh deleted the fix_kustomization_warnings branch May 21, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

kustomize v5 task: patchesStrategicMerge is deprecated
4 participants