From aaa3d754ce093c05f7ee202f7ffa4d2c9a5be0f2 Mon Sep 17 00:00:00 2001
From: Josh Deprez <jd@buildkite.com>
Date: Wed, 2 Aug 2023 09:58:25 +1000
Subject: [PATCH] Fix scalar plugin parsing

Because plugins is a list, each list item could be a single string and not necessarily a map.
---
 internal/pipeline/parser_test.go | 71 ++++++++++++++++++++++++++++++++
 internal/pipeline/plugin.go      | 10 ++++-
 internal/pipeline/plugins.go     | 37 ++++++++++++++---
 3 files changed, 110 insertions(+), 8 deletions(-)

diff --git a/internal/pipeline/parser_test.go b/internal/pipeline/parser_test.go
index 6b9138bc67..6e7419afec 100644
--- a/internal/pipeline/parser_test.go
+++ b/internal/pipeline/parser_test.go
@@ -1092,6 +1092,77 @@ steps:
 	}
 }
 
+func TestParserParsesScalarPlugins(t *testing.T) {
+	input := strings.NewReader(`---
+  steps:
+    - name: ":s3: xxx"
+      command: "script/buildkite/xxx.sh"
+      plugins:
+        - example-plugin#v1.0.0
+        - another-plugin#v0.0.1-beta43
+        - docker-compose#v2.5.1:
+            config: .buildkite/docker/docker-compose.yml
+`)
+
+	got, err := Parse(input)
+	if err != nil {
+		t.Fatalf("Parse(input) error = %v", err)
+	}
+
+	want := &Pipeline{
+		Steps: Steps{
+			&CommandStep{
+				Command: "script/buildkite/xxx.sh",
+				Plugins: Plugins{
+					{
+						Name: "example-plugin#v1.0.0",
+					},
+					{
+						Name: "another-plugin#v0.0.1-beta43",
+					},
+					{
+						Name: "docker-compose#v2.5.1",
+						Config: ordered.MapFromItems(
+							ordered.TupleSA{Key: "config", Value: ".buildkite/docker/docker-compose.yml"},
+						),
+					},
+				},
+				RemainingFields: map[string]any{
+					"name": ":s3: xxx",
+				},
+			},
+		},
+	}
+	if diff := cmp.Diff(got, want, cmp.Comparer(ordered.EqualSA)); diff != "" {
+		t.Errorf("parsed pipeline diff (-got +want):\n%s", diff)
+	}
+
+	gotJSON, err := json.MarshalIndent(got, "", "  ")
+	if err != nil {
+		t.Fatalf(`json.MarshalIndent(got, "", "  ") error = %v`, err)
+	}
+	const wantJSON = `{
+  "steps": [
+    {
+      "command": "script/buildkite/xxx.sh",
+      "name": ":s3: xxx",
+      "plugins": [
+        "example-plugin#v1.0.0",
+        "another-plugin#v0.0.1-beta43",
+        {
+          "docker-compose#v2.5.1": {
+            "config": ".buildkite/docker/docker-compose.yml"
+          }
+        }
+      ]
+    }
+  ]
+}`
+	if diff := cmp.Diff(string(gotJSON), wantJSON); diff != "" {
+		t.Errorf("marshalled JSON diff (-got +want):\n%s", diff)
+	}
+}
+
 func TestParserParsesConditionalWithEndOfLineAnchorDollarSign(t *testing.T) {
 	tests := []struct {
 		desc        string
diff --git a/internal/pipeline/plugin.go b/internal/pipeline/plugin.go
index 5059a4642a..3c0ec4e47a 100644
--- a/internal/pipeline/plugin.go
+++ b/internal/pipeline/plugin.go
@@ -23,15 +23,21 @@ type Plugin struct {
 	Config any
 }
 
-// MarshalJSON returns the plugin in "one-key object" form.
+// MarshalJSON returns the plugin in "one-key object" form, or "single string"
+// form (no config, only plugin name).
 func (p *Plugin) MarshalJSON() ([]byte, error) {
 	// NB: MarshalYAML (as seen below) never returns an error.
 	o, _ := p.MarshalYAML()
 	return json.Marshal(o)
 }
 
-// MarshalYAML returns the plugin in "one-item map" form.
+// MarshalYAML returns the plugin in either "one-item map" form, or "scalar"
+// form (no config, only plugin name).
 func (p *Plugin) MarshalYAML() (any, error) {
+	if p.Config == nil {
+		return p.Name, nil
+	}
+
 	return map[string]any{
 		p.Name: p.Config,
 	}, nil
diff --git a/internal/pipeline/plugins.go b/internal/pipeline/plugins.go
index 00977b2a9e..64e12f6d98 100644
--- a/internal/pipeline/plugins.go
+++ b/internal/pipeline/plugins.go
@@ -33,16 +33,41 @@ func (p *Plugins) unmarshalAny(o any) error {
 	switch o := o.(type) {
 	case []any:
 		for _, c := range o {
-			m, ok := c.(*ordered.MapSA)
-			if !ok {
-				return fmt.Errorf("unmarshaling plugins: plugin type %T, want *ordered.Map[string, any]", c)
-			}
-			if err := unmarshalMap(m); err != nil {
-				return err
+			switch ct := c.(type) {
+			case *ordered.MapSA:
+				// Typical case:
+				//
+				// plugins:
+				//   - plugin#1.0.0:
+				//       config: config, etc
+				if err := unmarshalMap(ct); err != nil {
+					return err
+				}
+
+			case string:
+				// Less typical, but supported:
+				//
+				// plugins:
+				//   - plugin#1.0.0
+				// (no config, only plugin)
+				*p = append(*p, &Plugin{
+					Name:   ct,
+					Config: nil,
+				})
+
+			default:
+				return fmt.Errorf("unmarshaling plugins: plugin type %T, want *ordered.Map[string, any] or string", c)
 			}
 		}
 
 	case *ordered.MapSA:
+		// Legacy form:
+		//
+		// plugins:
+		//   plugin#1.0.0:
+		//     config: config, etc
+		//   otherplugin#2.0.0:
+		//     etc
 		if err := unmarshalMap(o); err != nil {
 			return err
 		}