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.

Sign up for GitHub

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

Jump to bottom

add edit fix for patchesStrategicMerge to patches #4733

Merged
merged 8 commits into from
Oct 21, 2022
Merged

add edit fix for patchesStrategicMerge to patches #4733

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
64f42ea
add edit-fix for patchesStrategicMerge to patches
koba1t Jul 29, 2022
bb7ebe0
support file path patch to patchesStrategicMerge
koba1t Aug 6, 2022
3f0c213
refactor edit fix test cases
koba1t Aug 6, 2022
b4d25b1
add testcases
koba1t Aug 28, 2022
032bf33
update checker code in patchStrategicMerge
koba1t Aug 28, 2022
cbb61fc
fix yaml in test
koba1t Sep 1, 2022
7c2e884
fix test with t.Run()
koba1t Sep 12, 2022
0d68e0c
add newline for fix.go
koba1t Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions api/types/kustomization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"bytes"
"encoding/json"
"fmt"
"strings"

"sigs.k8s.io/yaml"
)
Expand Down Expand Up @@ -222,6 +223,21 @@ func (k *Kustomization) FixKustomizationPreMarshalling() error {
k.Patches = append(k.Patches, k.PatchesJson6902...)
k.PatchesJson6902 = nil

if k.PatchesStrategicMerge != nil {
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") {
Copy link
Contributor

@natasha41575 natasha41575 Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@koba1t koba1t Aug 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I rewrite at 032bf33.

// path patch
k.Patches = append(k.Patches, Patch{Path: string(patchStrategicMerge)})
} else {
// inline string patch
k.Patches = append(k.Patches, Patch{Patch: string(patchStrategicMerge)})
}
}
k.PatchesStrategicMerge = nil
}

// this fix is not in FixKustomizationPostUnmarshalling because
// it will break some commands like `create` and `add`. those
// commands depend on 'commonLabels' field
Expand Down
2 changes: 2 additions & 0 deletions kustomize/commands/edit/fix/fix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,14 @@ func RunFix(fSys filesys.FileSystem, w io.Writer) error {
fmt.Fprintln(w, `
Fixed fields:
patchesJson6902 -> patches
patchesStrategicMerge -> patches
commonLabels -> labels
vars -> replacements`)
} else {
fmt.Fprintln(w, `
Fixed fields:
patchesJson6902 -> patches
patchesStrategicMerge -> patches
commonLabels -> labels

To convert vars -> replacements, run the command `+"`kustomize edit fix --vars`"+`
Expand Down
234 changes: 176 additions & 58 deletions kustomize/commands/edit/fix/fix_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,15 @@ func TestFix(t *testing.T) {
assert.Contains(t, string(content), "kind: Kustomization")
}

func TestFixOutdatedPatchesFieldTitle(t *testing.T) {
kustomizationContentWithOutdatedPatchesFieldTitle := []byte(`
func TestFixCommand(t *testing.T) {
tests := []struct {
name string
input string
expected string
}{
{
name: "FixOutdatedPatchesFieldTitle",
input: `
patchesJson6902:
- path: patch1.yaml
target:
Expand All @@ -38,9 +45,8 @@ patchesJson6902:
group: apps
kind: Deployment
version: v1
`)

expected := []byte(`
`,
expected: `
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
Expand All @@ -52,24 +58,11 @@ patches:
group: apps
kind: Deployment
version: v1
`)
fSys := filesys.MakeFsInMemory()
testutils_test.WriteTestKustomizationWith(fSys, kustomizationContentWithOutdatedPatchesFieldTitle)
cmd := NewCmdFix(fSys, os.Stdout)
assert.NoError(t, cmd.RunE(cmd, nil))

content, err := testutils_test.ReadTestKustomization(fSys)
assert.NoError(t, err)
assert.Contains(t, string(content), "apiVersion: ")
assert.Contains(t, string(content), "kind: Kustomization")

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

func TestRenameAndKeepOutdatedPatchesField(t *testing.T) {
kustomizationContentWithOutdatedPatchesFieldTitle := []byte(`
`,
},
{
name: "TestRenameAndKeepOutdatedPatchesField",
input: `
patchesJson6902:
- path: patch1.yaml
target:
Expand All @@ -81,9 +74,8 @@ patches:
- path: patch3.yaml
target:
kind: Service
`)

expected := []byte(`
`,
expected: `
patches:
- path: patch2.yaml
target:
Expand All @@ -96,32 +88,154 @@ patches:
kind: Deployment
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
`)
fSys := filesys.MakeFsInMemory()
testutils_test.WriteTestKustomizationWith(fSys, kustomizationContentWithOutdatedPatchesFieldTitle)
cmd := NewCmdFix(fSys, os.Stdout)
assert.NoError(t, cmd.RunE(cmd, nil))

content, err := testutils_test.ReadTestKustomization(fSys)
assert.NoError(t, err)
assert.Contains(t, string(content), "apiVersion: ")
assert.Contains(t, string(content), "kind: Kustomization")

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

func TestFixOutdatedCommonLabels(t *testing.T) {
kustomizationContentWithOutdatedCommonLabels := []byte(`
`,
},
{
name: "TestFixOutdatedPatchesStrategicMergeFieldTitle",
input: `
patchesStrategicMerge:
- |-
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
spec:
template:
spec:
containers:
- name: nginx
image: nignx:latest
`,
expected: `
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- patch: |-
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
spec:
template:
spec:
containers:
- name: nginx
image: nignx:latest
`,
},
{
name: "TestFixAndMergeOutdatedPatchesStrategicMergeFieldTitle",
input: `
patchesStrategicMerge:
- |-
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
spec:
template:
spec:
containers:
- name: nginx
image: nignx:latest
patches:
- path: patch2.yaml
target:
kind: Deployment
`,
expected: `
patches:
- path: patch2.yaml
target:
kind: Deployment
- patch: |-
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
spec:
template:
spec:
containers:
- name: nginx
image: nignx:latest
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
`,
},
{
name: "TestFixOutdatedPatchesStrategicMergeToPathFieldTitle",
input: `
patchesStrategicMerge:
- deploy.yaml
`,
expected: `
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- path: deploy.yaml
`,
},
{
name: "TestFixOutdatedPatchesStrategicMergeToPathFieldYMLTitle",
input: `
patchesStrategicMerge:
- deploy.yml
`,
expected: `
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- path: deploy.yml
`,
},
{
name: "TestFixOutdatedPatchesStrategicMergeFieldPatchEndOfYamlTitle",
input: `
patchesStrategicMerge:
- |-
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
spec:
template:
spec:
containers:
- name: nginx
env:
- name: CONFIG_FILE_PATH
value: home.yaml
`,
expected: `
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- patch: |-
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
spec:
template:
spec:
containers:
- name: nginx
env:
- name: CONFIG_FILE_PATH
value: home.yaml
`,
},
{
name: "TestFixOutdatedCommonLabels",
input: `
commonLabels:
foo: bar
labels:
- pairs:
a: b
`)

expected := []byte(`
`,
expected: `
labels:
- pairs:
a: b
Expand All @@ -130,19 +244,23 @@ labels:
foo: bar
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
`)
fSys := filesys.MakeFsInMemory()
testutils_test.WriteTestKustomizationWith(fSys, kustomizationContentWithOutdatedCommonLabels)
cmd := NewCmdFix(fSys, os.Stdout)
assert.NoError(t, cmd.RunE(cmd, nil))
`,
},
}
for _, test := range tests {
koba1t marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@annasong20 annasong20 Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

@koba1t koba1t Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I fixed it at 7c2e884

fSys := filesys.MakeFsInMemory()
testutils_test.WriteTestKustomizationWith(fSys, []byte(test.input))
cmd := NewCmdFix(fSys, os.Stdout)
assert.NoError(t, cmd.RunE(cmd, nil))

content, err := testutils_test.ReadTestKustomization(fSys)
assert.NoError(t, err)
assert.Contains(t, string(content), "apiVersion: ")
assert.Contains(t, string(content), "kind: Kustomization")
content, err := testutils_test.ReadTestKustomization(fSys)
assert.NoError(t, err)
assert.Contains(t, string(content), "apiVersion: ")
assert.Contains(t, string(content), "kind: Kustomization")
koba1t marked this conversation as resolved.
Show resolved Hide resolved

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}
}

Expand Down