From 7a082cc2b640cd413311df2bdd4413c7ce295911 Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Fri, 15 Sep 2023 18:02:52 +0200 Subject: [PATCH 1/2] fix(package parser): only drop fully commented files Signed-off-by: Philippe Scorsolini --- pkg/parser/parser.go | 7 +--- pkg/parser/parser_test.go | 88 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 6 deletions(-) diff --git a/pkg/parser/parser.go b/pkg/parser/parser.go index b464ae82e..18c60332c 100644 --- a/pkg/parser/parser.go +++ b/pkg/parser/parser.go @@ -130,19 +130,16 @@ func (p *PackageParser) Parse(_ context.Context, reader io.ReadCloser) (*Package return pkg, nil } -// cleanYAML cleans up YAML by removing empty and commented out lines which +// cleanYAML cleans up YAML only drops fully commented inputs which would // cause issues with decoding. func cleanYAML(y []byte) []byte { lines := []string{} empty := true for _, line := range strings.Split(string(y), "\n") { trimmed := strings.TrimSpace(line) - if trimmed == "" || strings.HasPrefix(trimmed, "#") { - continue - } // We don't want to return an empty document with only separators that // have nothing in-between. - if empty && trimmed != "---" && trimmed != "..." { + if empty && trimmed != "" && trimmed != "---" && trimmed != "..." && !strings.HasPrefix(trimmed, "#") { empty = false } lines = append(lines, line) diff --git a/pkg/parser/parser_test.go b/pkg/parser/parser_test.go index 6d19698d4..64c7b137b 100644 --- a/pkg/parser/parser_test.go +++ b/pkg/parser/parser_test.go @@ -56,7 +56,12 @@ metadata: deployBytes = []byte(`apiVersion: apps/v1 kind: Deployment metadata: - name: test`) + name: test + annotations: + crossplane.io/managed: | + #!/bin/bash some script + some script +`) commentedOutBytes = []byte(`# apiVersion: apps/v1 # kind: Deployment @@ -210,3 +215,84 @@ func TestParser(t *testing.T) { }) } } + +func TestCleanYAML(t *testing.T) { + type args struct { + in []byte + } + type want struct { + out []byte + } + cases := map[string]struct { + reason string + args args + want want + }{ + "Empty": { + reason: "Should return nil on empty input", + args: args{in: []byte("")}, + want: want{out: nil}, + }, + "CommentedOut": { + reason: "Should return nil on a fully commented out input", + args: args{in: []byte(`# apiVersion: apps/v1 +# kind: Deployment +# metadata: +# name: test`)}, + want: want{out: nil}, + }, + "CommentedOutExceptSeparator": { + reason: "Should return nil on a fully commented out input", + args: args{in: []byte(`--- +# apiVersion: apps/v1 +# kind: Deployment +# metadata: +# name: test`)}, + want: want{out: nil}, + }, + "NotFullyCommentedOut": { + reason: "Should return the full file on a partially commented out input", + args: args{in: []byte(`--- +# some comment +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test`)}, + want: want{out: []byte(`--- +# some comment +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test`)}, + }, + "ShebangAnnotation": { + reason: "Should return the full file on a shebang annotation", + args: args{in: []byte(`--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test + annotations: + someScriptWithAShebang: | + #!/bin/bash + some script`)}, + want: want{out: []byte(`--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test + annotations: + someScriptWithAShebang: | + #!/bin/bash + some script`)}, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := cleanYAML(tc.args.in) + if diff := cmp.Diff(tc.want.out, got); diff != "" { + t.Errorf("cleanYAML: -want, +got:\n%s", diff) + } + }) + } +} From 6cb0b49702285a8dc528b75ca8fb9012fca4e028 Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Tue, 19 Sep 2023 12:53:41 +0200 Subject: [PATCH 2/2] refactor: switch to side effect free check Signed-off-by: Philippe Scorsolini --- pkg/parser/parser.go | 25 +++++++---------- pkg/parser/parser_test.go | 59 ++++++++++++++++++++++----------------- 2 files changed, 43 insertions(+), 41 deletions(-) diff --git a/pkg/parser/parser.go b/pkg/parser/parser.go index 18c60332c..ff30bd80a 100644 --- a/pkg/parser/parser.go +++ b/pkg/parser/parser.go @@ -22,6 +22,7 @@ import ( "context" "io" "strings" + "unicode" "github.com/spf13/afero" corev1 "k8s.io/api/core/v1" @@ -107,8 +108,7 @@ func (p *PackageParser) Parse(_ context.Context, reader io.ReadCloser) (*Package if errors.Is(err, io.EOF) { break } - content = cleanYAML(content) - if len(content) == 0 { + if isEmptyYAML(content) { continue } m, _, err := dm.Decode(content, nil, nil) @@ -130,24 +130,19 @@ func (p *PackageParser) Parse(_ context.Context, reader io.ReadCloser) (*Package return pkg, nil } -// cleanYAML cleans up YAML only drops fully commented inputs which would -// cause issues with decoding. -func cleanYAML(y []byte) []byte { - lines := []string{} - empty := true +// isEmptyYAML checks whether the provided YAML can be considered empty. This +// is useful for filtering out empty YAML documents that would otherwise +// cause issues when decoded. +func isEmptyYAML(y []byte) bool { for _, line := range strings.Split(string(y), "\n") { - trimmed := strings.TrimSpace(line) + trimmed := strings.TrimLeftFunc(line, unicode.IsSpace) // We don't want to return an empty document with only separators that // have nothing in-between. - if empty && trimmed != "" && trimmed != "---" && trimmed != "..." && !strings.HasPrefix(trimmed, "#") { - empty = false + if trimmed != "" && trimmed != "---" && trimmed != "..." && !strings.HasPrefix(trimmed, "#") { + return false } - lines = append(lines, line) } - if empty { - return nil - } - return []byte(strings.Join(lines, "\n")) + return true } // annotateErr annotates an error if the reader is an AnnotatedReadCloser. diff --git a/pkg/parser/parser_test.go b/pkg/parser/parser_test.go index 64c7b137b..c5d56d99d 100644 --- a/pkg/parser/parser_test.go +++ b/pkg/parser/parser_test.go @@ -221,7 +221,7 @@ func TestCleanYAML(t *testing.T) { in []byte } type want struct { - out []byte + out bool } cases := map[string]struct { reason string @@ -229,44 +229,59 @@ func TestCleanYAML(t *testing.T) { want want }{ "Empty": { - reason: "Should return nil on empty input", + reason: "Should return true on empty input", args: args{in: []byte("")}, - want: want{out: nil}, + want: want{out: true}, + }, + "EmptyLine": { + reason: "Should return true on an input with an empty line", + args: args{in: []byte("\n")}, + want: want{out: true}, + }, + "WhitespaceOnly": { + reason: "Should return true on an input with only whitespaces", + args: args{in: []byte(" \n\t ")}, + want: want{out: true}, + }, + "OnlyYAMLSeparators": { + reason: "Should return true on an input with only YAML separators", + args: args{in: []byte("---\n...")}, + want: want{out: true}, + }, + "YAMLWithWhitespaceLineAndNonEmptyLine": { + reason: "Should return false on having whitespace and non empty line in the input", + args: args{in: []byte(" \nkey: value")}, + want: want{out: false}, }, "CommentedOut": { - reason: "Should return nil on a fully commented out input", + reason: "Should return true on a fully commented out input", args: args{in: []byte(`# apiVersion: apps/v1 # kind: Deployment # metadata: # name: test`)}, - want: want{out: nil}, + want: want{out: true}, }, "CommentedOutExceptSeparator": { - reason: "Should return nil on a fully commented out input", + reason: "Should return true on a fully commented out input with a separator not commented", args: args{in: []byte(`--- # apiVersion: apps/v1 # kind: Deployment # metadata: # name: test`)}, - want: want{out: nil}, + want: want{out: true}, }, "NotFullyCommentedOut": { - reason: "Should return the full file on a partially commented out input", + reason: "Should return false on a partially commented out input", args: args{in: []byte(`--- # some comment apiVersion: apps/v1 kind: Deployment metadata: name: test`)}, - want: want{out: []byte(`--- -# some comment -apiVersion: apps/v1 -kind: Deployment -metadata: - name: test`)}, + want: want{out: false}, }, "ShebangAnnotation": { - reason: "Should return the full file on a shebang annotation", + reason: "Should return false with just a shebang annotation", args: args{in: []byte(`--- apiVersion: apps/v1 kind: Deployment @@ -276,22 +291,14 @@ metadata: someScriptWithAShebang: | #!/bin/bash some script`)}, - want: want{out: []byte(`--- -apiVersion: apps/v1 -kind: Deployment -metadata: - name: test - annotations: - someScriptWithAShebang: | - #!/bin/bash - some script`)}, + want: want{out: false}, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - got := cleanYAML(tc.args.in) + got := isEmptyYAML(tc.args.in) if diff := cmp.Diff(tc.want.out, got); diff != "" { - t.Errorf("cleanYAML: -want, +got:\n%s", diff) + t.Errorf("isEmptyYAML: -want, +got:\n%s", diff) } }) }