Merge pull request #548 from phisco/avoid-dropping-commented-lines

fix(package parser): only drop fully commented files

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,27 +130,19 @@ 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
-// 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)
-		if trimmed == "" || strings.HasPrefix(trimmed, "#") {
-			continue
-		}
+		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 != "..." {
-			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.

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,91 @@ func TestParser(t *testing.T) {
 		})
 	}
 }
+
+func TestCleanYAML(t *testing.T) {
+	type args struct {
+		in []byte
+	}
+	type want struct {
+		out bool
+	}
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"Empty": {
+			reason: "Should return true on empty input",
+			args:   args{in: []byte("")},
+			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 true on a fully commented out input",
+			args: args{in: []byte(`# apiVersion: apps/v1
+# kind: Deployment
+# metadata:
+#   name: test`)},
+			want: want{out: true},
+		},
+		"CommentedOutExceptSeparator": {
+			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: true},
+		},
+		"NotFullyCommentedOut": {
+			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: false},
+		},
+		"ShebangAnnotation": {
+			reason: "Should return false with just a shebang annotation",
+			args: args{in: []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 := isEmptyYAML(tc.args.in)
+			if diff := cmp.Diff(tc.want.out, got); diff != "" {
+				t.Errorf("isEmptyYAML: -want, +got:\n%s", diff)
+			}
+		})
+	}
+}