Merge pull request #156 from buildkite-plugins/SUP-5894-fix-notify-group-step

Fix `notify` attribute when step is inside `group` step

Author: petetomasik

pipeline_test.go

@@ -826,6 +826,39 @@ func TestGeneratePipelineWithSecretsInGroup(t *testing.T) {
 	assert.Contains(t, string(got), "- PROD_DB_PASS")
 }
 
+func TestGeneratePipelineWithNotifyInGroup(t *testing.T) {
+	steps := []Step{{
+		Group: "Test Group",
+		Steps: []Step{{
+			Label:   "Run Tests",
+			Command: "echo 'test'",
+			Notify: []StepNotify{{
+				GithubStatus: GithubStatusNotification{
+					Context: "buildkite/test/status",
+				},
+			}},
+		}},
+	}}
+
+	plugin := Plugin{}
+
+	tmp, hasPipeline, err := generatePipeline(steps, plugin)
+	assert.NoError(t, err)
+	assert.True(t, hasPipeline)
+	defer os.Remove(tmp.Name())
+
+	content, err := os.ReadFile(tmp.Name())
+	assert.NoError(t, err)
+
+	output := string(content)
+
+	// Verify correct notify syntax (not rawnotify)
+	assert.Contains(t, output, "notify:")
+	assert.NotContains(t, output, "rawnotify")
+	assert.Contains(t, output, "github_commit_status:")
+	assert.Contains(t, output, "context: buildkite/test/status")
+}
+
 func TestFilterValidSteps_AllValid(t *testing.T) {
 	steps := []Step{
 		{Command: "echo valid 1"},

plugin.go

@@ -421,13 +421,18 @@ func setBuild(build *Build) {
 	}
 }
 
-// processNestedStepsEnv recursively processes environment variables for nested steps
-func processNestedStepsEnv(steps []Step, env map[string]string) {
+// processNestedSteps recursively processes nested steps, handling environment variables and notify configurations
+func processNestedSteps(steps []Step, env map[string]string) {
 	for i := range steps {
 		// Parse the step's own env
 		steps[i].Env, _ = parseEnv(steps[i].RawEnv)
 		steps[i].Build.Env, _ = parseEnv(steps[i].Build.RawEnv)
 
+		// Process notify for nested steps
+		if steps[i].RawNotify != nil {
+			setNotify(&steps[i].Notify, &steps[i].RawNotify)
+		}
+
 		// Append top-level env to this step
 		for key, value := range env {
 			if steps[i].Command != nil || steps[i].Commands != nil {
@@ -449,7 +454,7 @@ func processNestedStepsEnv(steps []Step, env map[string]string) {
 
 		// Recursively process any nested steps
 		if len(steps[i].Steps) > 0 {
-			processNestedStepsEnv(steps[i].Steps, env)
+			processNestedSteps(steps[i].Steps, env)
 		}
 	}
 }
@@ -480,9 +485,9 @@ func appendEnv(watch *WatchConfig, env map[string]string) {
 
 	watch.Step.RawEnv = nil
 	watch.Step.Build.RawEnv = nil
-	// Process nested steps' environment variables
+	// Process nested steps
 	if len(watch.Step.Steps) > 0 {
-		processNestedStepsEnv(watch.Step.Steps, env)
+		processNestedSteps(watch.Step.Steps, env)
 	}
 	watch.RawPath = nil
 	watch.RawSkipPath = nil

plugin_test.go

@@ -943,6 +943,43 @@ func TestPluginShouldClearRawEnvFromNestedSteps(t *testing.T) {
 	assert.Nil(t, secondStep.Build.RawEnv, "Second nested step Build.RawEnv should be nil")
 }
 
+func TestPluginShouldProcessNotifyInNestedSteps(t *testing.T) {
+	param := `[{
+		"github.com/buildkite-plugins/monorepo-diff-buildkite-plugin#v1.0.0": {
+			"watch": [{
+				"path": "foo/**",
+				"config": {
+					"group": "Foo",
+					"steps": [{
+						"command": "./scripts/a.sh",
+						"label": "Run A",
+						"notify": [{
+							"github_commit_status": {
+								"context": "buildkite/foo/a"
+							}
+						}]
+					}]
+				}
+			}]
+		}
+	}]`
+
+	got, err := initializePlugin(param)
+	assert.NoError(t, err)
+
+	// Verify nested step exists
+	assert.Equal(t, 1, len(got.Watch[0].Step.Steps))
+
+	nestedStep := got.Watch[0].Step.Steps[0]
+
+	// Verify RawNotify is cleared
+	assert.Nil(t, nestedStep.RawNotify, "RawNotify should be nil after processing")
+
+	// Verify Notify is populated
+	assert.Equal(t, 1, len(nestedStep.Notify), "Notify should have 1 entry")
+	assert.Equal(t, "buildkite/foo/a", nestedStep.Notify[0].GithubStatus.Context)
+}
+
 func TestPluginShouldPreserveDependsOnString(t *testing.T) {
 	param := `[{
 		"github.com/buildkite-plugins/monorepo-diff-buildkite-plugin#commit": {
@@ -1331,8 +1368,8 @@ func TestParseEnvMapEmptyStringPreserved(t *testing.T) {
 	result, err := parseEnv(input)
 
 	assert.NoError(t, err)
-	assert.Equal(t, "", result["TEST_VAR"])        // Empty string preserved
-	assert.Equal(t, "", result["ANOTHER_VAR"])     // Empty string preserved
+	assert.Equal(t, "", result["TEST_VAR"])    // Empty string preserved
+	assert.Equal(t, "", result["ANOTHER_VAR"]) // Empty string preserved
 	assert.Equal(t, "value", result["EXPLICIT"])
 }
 
@@ -1663,8 +1700,8 @@ func TestMapEnvWithOSEnvReading(t *testing.T) {
 	got, err := initializePlugin(param)
 
 	assert.NoError(t, err)
-	assert.Equal(t, "us-west-2", got.Env["AWS_REGION"])  // Null reads from OS env
-	assert.Equal(t, "", got.Env["EMPTY_VAR"])            // Empty string is literal
+	assert.Equal(t, "us-west-2", got.Env["AWS_REGION"]) // Null reads from OS env
+	assert.Equal(t, "", got.Env["EMPTY_VAR"])           // Empty string is literal
 	assert.Equal(t, "explicit-value", got.Env["EXPLICIT"])
 }