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

Change kustomize edit fix to split patchesStrategicMerge entries before converting them to patch entries. #5059

Closed

Conversation

brianpursley
Copy link
Member

@brianpursley brianpursley commented Feb 22, 2023

Change kustomize edit fix to split patchesStrategicMerge entries before converting them to patch entries.

Fixes #5040

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 22, 2023
@brianpursley brianpursley force-pushed the kustomize-5040 branch 2 times, most recently from 8013049 to 27d2858 Compare February 22, 2023 02:37
kustomize/commands/edit/fix/splitpatches.go Outdated Show resolved Hide resolved
kustomize/commands/edit/fix/fix.go Outdated Show resolved Hide resolved
@brianpursley brianpursley changed the title Change kustomize edit fix to split patch files that contain multiple patches Change kustomize edit fix to split patchesStrategicMerge entries before converting them to patch entries. Mar 7, 2023
@brianpursley brianpursley force-pushed the kustomize-5040 branch 3 times, most recently from 9cd9a24 to d7510ea Compare March 8, 2023 02:39
@brianpursley
Copy link
Member Author

Hmm, I think there might be something wrong with the GitHub action. make lint succeeds locally, but is "terminated" without a descriptive error when run on GitHub.

----------------------------------------------------------
Running command in /api
----------------------------------------------------------
make[1]: Entering directory '/home/runner/work/kustomize/kustomize/api'
Makefile:7: warning: overriding recipe for target 'test'
../Makefile-modules.mk:23: warning: ignoring old recipe for target 'test'
Makefile:10: warning: overriding recipe for target 'build'
../Makefile-modules.mk:38: warning: ignoring old recipe for target 'build'
/home/runner/go/bin/golangci-lint \
  -c $KUSTOMIZE_ROOT/.golangci.yml \
  --path-prefix api \
  run ./...
make[1]: *** [../Makefile-modules.mk:17: lint] Terminated
make: *** wait: No child processes.  Stop.
make: *** Waiting for unfinished jobs....
make: *** wait: No child processes.  Stop.
Error: The operation was canceled.

@koba1t
Copy link
Member

koba1t commented Mar 8, 2023

@brianpursley

Hmm, I think there might be something wrong with the GitHub action. make lint succeeds locally, but is "terminated" without a descriptive error when run on GitHub

This lint failure may fix after doing rebase master. I found the same problem on a few PRs.
I'm guessing Github Actions exec merge master before running CI.

So if this lint error happened after the merge process, rebase master is needed to find problems.

@brianpursley
Copy link
Member Author

Ah, thanks @koba1t

That makes sense. I'll give that a try

@koba1t
Copy link
Member

koba1t commented Mar 8, 2023

Hmm...
My PR updating golangci-lint passed the lint check.

@koba1t
Copy link
Member

koba1t commented Mar 8, 2023

So, Maybe this lint failure was caused by a down for GitHub Actions.
Could you try to re-run actions?

If that lint fails again, we update golangci-lint quickly.

@koba1t
Copy link
Member

koba1t commented Mar 11, 2023

Hi @brianpursley
My PR is merged.
Could you retry rebase master?

@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

@brianpursley
Copy link
Member Author

/assign @KnVerey

@koba1t
Copy link
Member

koba1t commented Mar 29, 2023

/triage accepted
/ok-to-test
/priority important-soon
/kind feature

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 29, 2023
api/types/kustomization.go Outdated Show resolved Hide resolved
api/types/kustomization.go Outdated Show resolved Hide resolved
api/types/kustomization.go Outdated Show resolved Hide resolved
api/types/kustomization.go Outdated Show resolved Hide resolved
kyaml/kio/byteio_reader.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2023
@pre
Copy link

pre commented May 31, 2023

Could this PR receive some attention and love?

We have quite a big manifest repository and our linting is currently swamped with the warnings from

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

Getting rid of the warning would be great, but splitting the files manually is busywork if multi-file support for kustomize edit fix is just around the corner.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brianpursley
Once this PR has been reviewed and has the lgtm label, please ask for approval from knverey. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@brianpursley
Copy link
Member Author

@pre sorry this fell off my radar. I made some changes and resolved the open comments. We'll see if we can get this back on track.

@brianpursley
Copy link
Member Author

/assign @natasha41575

@pre
Copy link

pre commented Jun 27, 2023

tide Pending — Not mergeable. Needs approved, lgtm labels

Otherwise OK ?

@stormqueen1990
Copy link
Member

/cc

@stormqueen1990
Copy link
Member

Code-wise, this looks good to me, but it looks like merging #5194 also fixed #5040. I tried to reproduce the issue with the latest release of kustomize (v5.2.1, released on Oct 19th) and was unable to.

@natasha41575
Copy link
Contributor

@stormqueen1990 Thanks for taking a look!

It looks like #5194 changes patchesStrategicMerge to accept a file that contains multiple patches. Meaning that when you run kustomize edit fix to convert patches to patchesStrategicMerge, it no longer throws an error when you have a file with multiple patches.

In that case, I think this PR is not needed anymore, @brianpursley can you please confirm before we close this?

@natasha41575
Copy link
Contributor

@brianpursley closing this per my understanding in #5059 (comment), please LMK if I misunderstood something and you'd like to reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Development

Successfully merging this pull request may close these issues.

kustomize edit fix needs to split multi-doc SMP patches into multiple files
7 participants