Modified code for patches and patchJson6902 which will throw error if target is not found #4715

laxmikantbpandhare · 2022-07-17T23:27:42Z

Fix for patchesJson6902, patches is completed. This fix verifies whether is resources filed has any data or not. If it is empty then there is definitely a missing target.

patchStrategicMerge is already throwing an error if the target is missing.

This will fix #4379

k8s-ci-robot · 2022-07-17T23:27:49Z

Welcome @laxmikantbpandhare!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-07-17T23:27:50Z

Hi @laxmikantbpandhare. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2022-07-18T01:44:45Z

@laxmikantbpandhare: 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.

laxmikantbpandhare · 2022-07-18T06:27:08Z

/label tide/merge-method-squash

laxmikantbpandhare · 2022-07-18T06:27:50Z

/label WIP

k8s-ci-robot · 2022-07-18T06:27:51Z

@laxmikantbpandhare: The label(s) /label WIP cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor

In response to this:

/label WIP

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.

api/internal/builtins/PatchJson6902Transformer.go

KnVerey · 2022-07-19T21:19:22Z

/ok-to-test

k8s-ci-robot · 2022-08-01T23:36:39Z

@yuwenma: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test kustomize-presubmit-master

Use /test all to run all jobs.

In response to this:

/test

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.

yuwenma · 2022-08-01T23:38:32Z

/approve

yuwenma · 2022-08-01T23:39:04Z

/assign @natasha41575

laxmikantbpandhare · 2022-08-09T17:36:28Z

@KnVerey @natasha41575 - Could you please take a look at this.

annasong20 · 2023-03-23T01:14:26Z

/remove-lifecycle stale

laxmikantbpandhare · 2023-06-16T13:40:19Z

@laxmikantbpandhare Sorry for taking so long to get back to you. We just re-discussed #4379 and concluded #4379 (comment). If you'd like to continue the implementation in this PR, I'll try my best to give you timely reviews.

@annasong20 - thank you for your comment. I will start looking into it.

laxmikantbpandhare · 2023-07-23T17:31:53Z

As of now, Added as a log warning. I would like to know more about the stderr requirement for patchJsonTransformer. patchjson6902Transformer I kept it as it is as it ll be deprecated soon.

k8s-ci-robot · 2023-07-23T17:32:20Z

@laxmikantbpandhare: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
kustomize-presubmit-master	`0c5e93e`	link	true	`/test kustomize-presubmit-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

laxmikantbpandhare · 2023-07-23T17:33:14Z

Test Jobs are failing as test cases have not been updated yet.

annasong20

Thank you for all of your work on this, @laxmikantbpandhare!

I gave a high-level review of the current code behavior. Lmk if anything is unclear. Once we've fulfilled the behavior agreed upon in #4379 and added tests, I'll do another pass for details.

annasong20 · 2023-07-29T01:38:46Z

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour
 	if err != nil {
 		return err
 	}
+
+	if p.Options["allowNoTargetMatch"] {
+		log.Println("Warning: patches target not found for Target")


Can we print to os.Stderr instead? You can use fmt.Fprintf.

Updated as below:

fmt.Fprintf(os.Stderr, "%v\n", "Warning: patches target not found for Target")

I don't see this change in the code.

annasong20 · 2023-07-29T01:43:51Z

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour
 	if err != nil {
 		return err
 	}
+
+	if p.Options["allowNoTargetMatch"] {


This is close. Instead, we want to emit this warning if allowNoTargetMatch is true AND there is no matching target (selected is empty).

totally agree.

Updated as below:

if p.Options["allowNoTargetMatch"] && len(selected) == 0 {

I see your comment, but this doesn't seem to be reflected in the code? Can you commit your changes?

annasong20 · 2023-07-29T01:44:45Z

plugin/builtin/patchtransformer/PatchTransformer.go

+	if p.Options["allowNoTargetMatch"] {
+		log.Println("Warning: patches target not found for Target")
+	}


Same as above.

Updated same as above.

Don't see the change in code.

annasong20 · 2023-07-29T01:50:15Z

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour
 	if err != nil {
 		return err
 	}
+
+	if p.Options["allowNoTargetMatch"] {


Now that options is growing, can we define a struct under api/types to replace the map[string]bool? The field will be better documented that way and we'll be less likely to suffer from typo bugs.

@annasong20 - We can create a PatchTransformer struct under api/types. So, do you mean only the options field can be moved out of it and add it in api/types? Please let me know if I am wrong.

Good question! I only need options to be a struct under api/types, but if you'd like you can move the PatchTransformer arguments into a PatchArgs. The latter is more work, but I think easier to read in the long run.

plugin/builtin/patchjson6902transformer/PatchJson6902Transformer.go

k8s-ci-robot · 2023-09-16T05:53:45Z

PR needs rebase.

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.

annasong20 · 2023-10-20T21:28:53Z

/remove-approve

annasong20 · 2023-10-20T21:55:26Z

/unhold

annasong20

@laxmikantbpandhare Sorry for taking so long to re-review. I took a closer look this time. Can you address my new comments?

Also, could you rebase?

annasong20 · 2023-10-20T23:06:05Z

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour
 	if err != nil {
 		return err
 	}
+
+	if p.Options["allowNoTargetMatch"] {


Good question! I only need options to be a struct under api/types, but if you'd like you can move the PatchTransformer arguments into a PatchArgs. The latter is more work, but I think easier to read in the long run.

annasong20 · 2023-10-20T23:06:42Z

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour
 	if err != nil {
 		return err
 	}
+
+	if p.Options["allowNoTargetMatch"] {


I see your comment, but this doesn't seem to be reflected in the code? Can you commit your changes?

annasong20 · 2023-10-20T23:21:47Z

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour
 	if err != nil {
 		return err
 	}
+
+	if p.Options["allowNoTargetMatch"] {
+		log.Println("Warning: patches target not found for Target")


I don't see this change in the code.

annasong20 · 2023-10-20T23:22:47Z

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour
 	if err != nil {
 		return err
 	}
+
+	if p.Options["allowNoTargetMatch"] {
+		log.Println("Warning: patches target not found for Target")


Can we print for users the "Target" that we're looking for?

As a user with a complex Kustomization, I'd be confused and need to debug which patch target is missing.

annasong20 · 2023-10-20T23:23:04Z

plugin/builtin/patchtransformer/PatchTransformer.go

+	if p.Options["allowNoTargetMatch"] {
+		log.Println("Warning: patches target not found for Target")
+	}


Don't see the change in code.

annasong20 · 2023-10-20T23:23:59Z

plugin/builtin/patchtransformer/PatchTransformer.go

+
+	if p.Options["allowNoTargetMatch"] {
+		log.Println("Warning: patches target not found for Target")
+	}


We still need to return an error if allowNoTargetMatch is not set?

annasong20 · 2023-10-20T23:30:34Z

api/krusty/extendedpatch_test.go

+    version: v2
+  path: patch.0.yaml
+`)
+	th.WriteF("base/servicemonitor.yaml", `


Can we use the existing files in makeCommonFileForExtendedPatchTest for this test?

annasong20 · 2023-10-20T23:31:50Z

api/krusty/extendedpatch_test.go

@@ -1213,3 +1066,44 @@ spec:
    app: busybox
 `)
 }
+
+func TestTargetMissingPatchJson6902Error(t *testing.T) {


We don't seem to have tests for

allowNoTargetMatch

json patches under the patches field

natasha41575 · 2023-10-23T15:23:52Z

/approve cancel

k8s-ci-robot · 2023-10-23T15:23:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: laxmikantbpandhare, yuwenma
Once this PR has been reviewed and has the lgtm label, please ask for approval from natasha41575. 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wibed · 2023-11-30T13:31:26Z

@laxmikantbpandhare

selfish bump, looking forward to it

k8s-triage-robot · 2024-02-28T14:16:28Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

annasong20 · 2024-02-28T18:47:39Z

/unassign

annasong20 · 2024-02-28T18:48:32Z

/uncc

k8s-triage-robot · 2024-03-29T18:54:03Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-04-28T19:53:47Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-04-28T19:53:52Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 17, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 17, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 17, 2022

k8s-ci-robot requested review from KnVerey and yuwenma July 17, 2022 23:28

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 18, 2022

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 18, 2022

yuwenma reviewed Jul 18, 2022

View reviewed changes

api/internal/builtins/PatchJson6902Transformer.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 19, 2022

laxmikantbpandhare requested a review from yuwenma July 19, 2022 03:00

laxmikantbpandhare force-pushed the remove-var-ref branch from 19c4c7a to 941a44b Compare July 19, 2022 06:36

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 19, 2022

k8s-ci-robot assigned yuwenma Aug 1, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2022

k8s-ci-robot assigned natasha41575 Aug 1, 2022

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 23, 2023

updated error message according to new discussion

0c5e93e

annasong20 suggested changes Jul 29, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2023

annasong20 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2023

annasong20 suggested changes Oct 20, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2023

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 28, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 29, 2024

k8s-ci-robot closed this Apr 28, 2024

Modified code for patches and patchJson6902 which will throw error if target is not found #4715

Modified code for patches and patchJson6902 which will throw error if target is not found #4715

Conversation

laxmikantbpandhare commented Jul 17, 2022 • edited

k8s-ci-robot commented Jul 17, 2022

k8s-ci-robot commented Jul 17, 2022

k8s-ci-robot commented Jul 18, 2022

laxmikantbpandhare commented Jul 18, 2022

laxmikantbpandhare commented Jul 18, 2022

k8s-ci-robot commented Jul 18, 2022

KnVerey commented Jul 19, 2022

k8s-ci-robot commented Aug 1, 2022

yuwenma commented Aug 1, 2022

yuwenma commented Aug 1, 2022

laxmikantbpandhare commented Aug 9, 2022

annasong20 commented Mar 23, 2023

laxmikantbpandhare commented Jun 16, 2023

laxmikantbpandhare commented Jul 23, 2023

k8s-ci-robot commented Jul 23, 2023

laxmikantbpandhare commented Jul 23, 2023 • edited

annasong20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 16, 2023

annasong20 commented Oct 20, 2023

annasong20 commented Oct 20, 2023

annasong20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

wibed commented Nov 30, 2023

k8s-triage-robot commented Feb 28, 2024

annasong20 commented Feb 28, 2024

annasong20 commented Feb 28, 2024

k8s-triage-robot commented Mar 29, 2024

k8s-triage-robot commented Apr 28, 2024

k8s-ci-robot commented Apr 28, 2024

laxmikantbpandhare commented Jul 17, 2022 •

edited

laxmikantbpandhare commented Jul 23, 2023 •

edited