add `edit fix` for patchesStrategicMerge to patches #4733

koba1t · 2022-07-29T17:38:19Z

koba1t · 2022-07-29T18:49:32Z

k8s-ci-robot · 2022-08-06T20:02:55Z

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

natasha41575

So sorry for the long delay. Thank you for this change! Some minor comments

natasha41575 · 2022-08-25T16:02:41Z

api/types/kustomization.go

+		for _, patchStrategicMerge := range k.PatchesStrategicMerge {
+			// check this patch is file path select.
+			if strings.Count(string(patchStrategicMerge), "\n") < 1 &&
+				(patchStrategicMerge[len(patchStrategicMerge)-5:] == ".yaml" || patchStrategicMerge[len(patchStrategicMerge)-4:] == ".yml") {


An easier and more reliable way to check if this is a filepath or not is to try to open it (i.e. os.ReadFile), and check for error

I rewrite at 032bf33.

kustomize/commands/edit/fix/fix_test.go

koba1t · 2022-08-28T17:50:28Z

hi @natasha41575
I fixed a few points from the comments.
Could you check it if you have time?

natasha41575

LGTM. Very small nit about some formatting in the tests

kustomize/commands/edit/fix/fix_test.go

natasha41575 · 2022-09-01T16:48:36Z

kustomize/commands/edit/fix/fix_test.go

+spec:
+template:
+spec:
+containers:


same here (and a few other places)

I know it doesn't really matter for these tests, but it's good to make sure our tests have valid input

I know it doesn't really matter for these tests, but it's good to make sure our tests have valid input

Sorry, I couldn't find that before committing.
I completely agree with your opinion.

I fixed everything.

koba1t · 2022-09-01T22:50:37Z

hi @natasha41575
I fixed a few points.
Please recheck it in your free time.

natasha41575 · 2022-09-09T15:29:49Z

@annasong20 Do you mind giving this PR a review? And as a final check, it would be helpful if you could pull down and build the PR and verify that it does the right thing. Btw, when you are testing, it is expected that it won't work unless the patch file exists.

annasong20

@koba1t I know you didn't change this line, but could you add a newline to the end of it?

annasong20 · 2022-09-12T16:27:21Z

api/types/kustomization.go

+	if k.PatchesStrategicMerge != nil {
+		for _, patchStrategicMerge := range k.PatchesStrategicMerge {
+			// check this patch is file path select.
+			if _, err := fSys.ReadFile(string(patchStrategicMerge)); err == nil {


You don't need to change anything, but just as an FYI, the existing PatchesStrategicMerge code differentiates between inline and files like this: https://github.com/kubernetes-sigs/kustomize/blob/master/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go#L63
Checking for inlines this way doesn't require the file to exist.

annasong20 · 2022-09-12T16:45:51Z

kustomize/commands/edit/fix/fix_test.go

+`,
+		},
+	}
+	for _, test := range tests {


Can we move the loop body into the body of a function passed to t.Run()? The Run method runs its parameter function as a sub-test, which will display the results as such and isolate failures between sub-tests. We can also pass in test.name.

Very thorough testing!

I fixed it at 7c2e884

annasong20 · 2022-09-12T16:50:34Z

kustomize/commands/edit/fix/fix_test.go

+		if diff := cmp.Diff([]byte(test.expected), content); diff != "" {
+			t.Errorf("%s: Mismatch (-expected, +actual):\n%s", test.name, diff)
+		}


Optional nit: We can replace these lines with require.Empty(t, cmp.Diff([]byte(test.expected), content)) since t.Run() will print the sub-test name and require.Empty() will print diff if it isn't empty.

annasong20 · 2022-09-12T17:45:42Z

@natasha41575 The PR binary works as expected on the following setup:

kustomization.yaml

patchesStrategicMerge:
- deploy.yaml
- |-
  apiVersion: apps/v1
  kind: Deployment
  metadata:
    name: nginx
  spec:
    template:
      spec:
        containers:
          - name: nginx
            env:
              - name: CONFIG_FILE_PATH
                value: home.yaml
patchesJson6902:
- path: patch1.yaml
  target:
    kind: Deployment
patches:
- path: patch2.yaml
- patch: |-
    apiVersion: apps/v1
    kind: Deployment
    metadata:
      name: nginx
    spec:
      template:
        spec:
          containers:
            - name: nginx
              env:
                - name: CONFIG_FILE_PATH_2
                  value: home.yaml

deploy.yaml

apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx
spec:
  template:
    spec:
      containers:
      - name: nginx
        env:
        - name: CONFIG_FILE_PATH_3
          value: home.yaml

On a different note, I think it'd be helpful for the current code in fix_test.go to check for more than just an error. We should care about warnings that the build failed or that the build output is different. I know they're currently printed because we don't provide resources that match the patch targets, but the warnings could technically be the result of us "fixing" the kustomization incorrectly. It doesn't seem safe to ignore them.

Something else that I noticed while testing the binary, but of lesser importance, is that we don't inform users if their initial build was faulty. I found this very confusing because when I saw this warning, I'd assume edit fix introduced an inconsistency to my kustomization, but it turns out that it was the original kustomization that had issues.

Not saying we need to address these concerns in this PR, since the code wasn't introduced here.

koba1t · 2022-09-12T18:09:25Z

I know you didn't change this line, but could you add a newline to the end of it?

Do you think this commit fixes that?

koba1t · 2022-09-12T18:09:42Z

Hi, @annasong20
Thanks for your review.
I think I completed fixing the problems from your comment. Please recheck it when you have time.

annasong20 · 2022-09-14T06:50:27Z

I know you didn't change this line, but could you add a newline to the end of it?

Do you think this commit fixes that?

Yes, perfect! Looks good to me. Will defer to @natasha41575 to approve.

koba1t · 2022-10-12T17:02:24Z

@natasha41575
Could you recheck it?

Hi, @annasong20
Thanks for your review.

annasong20 · 2022-10-12T17:15:41Z

/lgtm

natasha41575 · 2022-10-21T21:41:28Z

So sorry for the delay! Reviewing this now

natasha41575 · 2022-10-21T21:47:53Z

Thanks so much for the fix! This means we can get #4723 in soon as well. This is great. Please feel free to reach out if you are looking for more issues to pick up.

/approve

k8s-ci-robot · 2022-10-21T21:48:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: koba1t, natasha41575

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~~ [natasha41575]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 29, 2022

k8s-ci-robot requested review from KnVerey and natasha41575 July 29, 2022 17:38

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 29, 2022

koba1t force-pushed the feat/add_edit-fix_for_patchesStrategicMerge_to_patches branch from ee59de2 to ab23987 Compare July 29, 2022 17:55

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

koba1t mentioned this pull request Jul 29, 2022

Emit a warning on build when deprecated fields are used #4723

Merged

k8s-ci-robot assigned natasha41575 Jul 29, 2022

koba1t added 2 commits August 6, 2022 20:28

add edit-fix for patchesStrategicMerge to patches

64f42ea

support file path patch to patchesStrategicMerge

bb7ebe0

koba1t force-pushed the feat/add_edit-fix_for_patchesStrategicMerge_to_patches branch from 17f2f48 to f1b15e3 Compare August 6, 2022 20:29

refactor edit fix test cases

3f0c213

koba1t force-pushed the feat/add_edit-fix_for_patchesStrategicMerge_to_patches branch from f1b15e3 to 3f0c213 Compare August 6, 2022 20:34

mythi mentioned this pull request Aug 25, 2022

Replace obsolete patches syntax intel/intel-device-plugins-for-kubernetes#1117

Merged

natasha41575 reviewed Aug 25, 2022

View reviewed changes

add testcases

b4d25b1

koba1t force-pushed the feat/add_edit-fix_for_patchesStrategicMerge_to_patches branch from a01afa9 to 7081b22 Compare August 28, 2022 17:14

update checker code in patchStrategicMerge

032bf33

koba1t force-pushed the feat/add_edit-fix_for_patchesStrategicMerge_to_patches branch from 7081b22 to 032bf33 Compare August 28, 2022 17:20

natasha41575 reviewed Sep 1, 2022

View reviewed changes

fix yaml in test

cbb61fc

annasong20 reviewed Sep 12, 2022

View reviewed changes

fix test with t.Run()

7c2e884

add newline for fix.go

0d68e0c

annasong20 mentioned this pull request Sep 28, 2022

REQUEST: New membership for @annasong20 kubernetes/org#3736

Closed

9 tasks

k8s-ci-robot assigned annasong20 Oct 12, 2022

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

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

k8s-ci-robot merged commit 7ee6dd5 into kubernetes-sigs:master Oct 21, 2022

koba1t deleted the feat/add_edit-fix_for_patchesStrategicMerge_to_patches branch October 21, 2022 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `edit fix` for patchesStrategicMerge to patches #4733

add `edit fix` for patchesStrategicMerge to patches #4733

koba1t commented Jul 29, 2022

koba1t commented Jul 29, 2022

k8s-ci-robot commented Aug 6, 2022

natasha41575 left a comment

natasha41575 Aug 25, 2022 •

edited

Loading

koba1t Aug 28, 2022 •

edited

Loading

koba1t commented Aug 28, 2022

natasha41575 left a comment

natasha41575 Sep 1, 2022

koba1t Sep 1, 2022

koba1t commented Sep 1, 2022

natasha41575 commented Sep 9, 2022 •

edited

Loading

annasong20 left a comment

annasong20 Sep 12, 2022

annasong20 Sep 12, 2022 •

edited

Loading

koba1t Sep 12, 2022 •

edited

Loading

annasong20 Sep 12, 2022

koba1t Sep 12, 2022

annasong20 commented Sep 12, 2022

koba1t commented Sep 12, 2022

koba1t commented Sep 12, 2022

annasong20 commented Sep 14, 2022

koba1t commented Oct 12, 2022

annasong20 commented Oct 12, 2022

natasha41575 commented Oct 21, 2022

natasha41575 commented Oct 21, 2022

k8s-ci-robot commented Oct 21, 2022

add edit fix for patchesStrategicMerge to patches #4733

add edit fix for patchesStrategicMerge to patches #4733

Conversation

koba1t commented Jul 29, 2022

koba1t commented Jul 29, 2022

k8s-ci-robot commented Aug 6, 2022

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

koba1t Aug 28, 2022 • edited Loading

Choose a reason for hiding this comment

koba1t commented Aug 28, 2022

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 Sep 1, 2022

Choose a reason for hiding this comment

koba1t Sep 1, 2022

Choose a reason for hiding this comment

koba1t commented Sep 1, 2022

natasha41575 commented Sep 9, 2022 • edited Loading

annasong20 left a comment

Choose a reason for hiding this comment

annasong20 Sep 12, 2022

Choose a reason for hiding this comment

annasong20 Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

koba1t Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

annasong20 Sep 12, 2022

Choose a reason for hiding this comment

koba1t Sep 12, 2022

Choose a reason for hiding this comment

annasong20 commented Sep 12, 2022

koba1t commented Sep 12, 2022

koba1t commented Sep 12, 2022

annasong20 commented Sep 14, 2022

koba1t commented Oct 12, 2022

annasong20 commented Oct 12, 2022

natasha41575 commented Oct 21, 2022

natasha41575 commented Oct 21, 2022

k8s-ci-robot commented Oct 21, 2022

add `edit fix` for patchesStrategicMerge to patches #4733

add `edit fix` for patchesStrategicMerge to patches #4733

natasha41575 Aug 25, 2022 •

edited

Loading

koba1t Aug 28, 2022 •

edited

Loading

natasha41575 commented Sep 9, 2022 •

edited

Loading

annasong20 Sep 12, 2022 •

edited

Loading

koba1t Sep 12, 2022 •

edited

Loading