Skip to content

Commit 1047435

Browse files
mcnclmcncl
authored
Merge pull request #102 from buildkite-plugins/SUP-4634
fix: nested envs parsing as rawenv
2 parents f8981c0 + 27a8229 commit 1047435

File tree

3 files changed

+139
-10
lines changed

3 files changed

+139
-10
lines changed

‎pipeline_test.go‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ func TestUploadPipelineCallsBuildkiteAgentCommand(t *testing.T) {
2828
}
2929

3030
oldPath := os.Getenv("PATH")
31-
t.Cleanup(func() { os.Setenv("PATH", oldPath) })
32-
os.Setenv("PATH", filepath.Dir(agent.Path)+":"+oldPath)
31+
t.Cleanup(func() { _ = os.Setenv("PATH", oldPath) })
32+
_ = os.Setenv("PATH", filepath.Dir(agent.Path)+":"+oldPath)
3333

3434
agent.
3535
Expect("pipeline", "upload", "pipeline.txt").
@@ -55,8 +55,8 @@ func TestUploadPipelineCallsBuildkiteAgentCommandWithInterpolation(t *testing.T)
5555
}
5656

5757
oldPath := os.Getenv("PATH")
58-
t.Cleanup(func() { os.Setenv("PATH", oldPath) })
59-
os.Setenv("PATH", filepath.Dir(agent.Path)+":"+oldPath)
58+
t.Cleanup(func() { _ = os.Setenv("PATH", oldPath) })
59+
_ = os.Setenv("PATH", filepath.Dir(agent.Path)+":"+oldPath)
6060

6161
agent.
6262
Expect("pipeline", "upload", "pipeline.txt", "--no-interpolation").
@@ -459,7 +459,7 @@ func TestGeneratePipeline(t *testing.T) {
459459

460460
require.NoError(t, err)
461461
defer func() {
462-
if err := os.Remove(pipeline.Name()); err != nil {
462+
if err = os.Remove(pipeline.Name()); err != nil {
463463
t.Logf("Failed to remove temporary pipeline file: %v", err)
464464
}
465465
}()
@@ -522,7 +522,11 @@ func TestGeneratePipelineWithNoStepsAndHooks(t *testing.T) {
522522

523523
pipeline, _, err := generatePipeline(steps, plugin)
524524
require.NoError(t, err)
525-
defer os.Remove(pipeline.Name())
525+
defer func() {
526+
if err = os.Remove(pipeline.Name()); err != nil {
527+
t.Logf("failed to remove teme file: %v", err)
528+
}
529+
}()
526530

527531
got, err := os.ReadFile(pipeline.Name())
528532
require.NoError(t, err)
@@ -540,7 +544,11 @@ func TestGeneratePipelineWithNoStepsAndNoHooks(t *testing.T) {
540544

541545
pipeline, _, err := generatePipeline(steps, plugin)
542546
require.NoError(t, err)
543-
defer os.Remove(pipeline.Name())
547+
defer func() {
548+
if err = os.Remove(pipeline.Name()); err != nil {
549+
t.Logf("Failed to remove temporary pipeline file: %v", err)
550+
}
551+
}()
544552

545553
got, err := os.ReadFile(pipeline.Name())
546554
require.NoError(t, err)
@@ -573,7 +581,11 @@ func TestGeneratePipelineWithCondition(t *testing.T) {
573581

574582
pipeline, _, err := generatePipeline(steps, plugin)
575583
require.NoError(t, err)
576-
defer os.Remove(pipeline.Name())
584+
defer func() {
585+
if err = os.Remove(pipeline.Name()); err != nil {
586+
t.Logf("Failed to remove temporary pipeline file: %v", err)
587+
}
588+
}()
577589

578590
got, err := os.ReadFile(pipeline.Name())
579591
require.NoError(t, err)

‎plugin.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,39 @@ func setBuild(build *Build) {
346346
}
347347
}
348348

349+
// processNestedStepsEnv recursively processes environment variables for nested steps
350+
func processNestedStepsEnv(steps []Step, env map[string]string) {
351+
for i := range steps {
352+
// Parse the step's own env
353+
steps[i].Env, _ = parseEnv(steps[i].RawEnv)
354+
steps[i].Build.Env, _ = parseEnv(steps[i].Build.RawEnv)
355+
356+
// Append top-level env to this step
357+
for key, value := range env {
358+
if steps[i].Command != nil || steps[i].Commands != nil {
359+
if steps[i].Env == nil {
360+
steps[i].Env = make(map[string]string)
361+
}
362+
steps[i].Env[key] = value
363+
} else if steps[i].Trigger != "" {
364+
if steps[i].Build.Env == nil {
365+
steps[i].Build.Env = make(map[string]string)
366+
}
367+
steps[i].Build.Env[key] = value
368+
}
369+
}
370+
371+
// Clear RawEnv fields
372+
steps[i].RawEnv = nil
373+
steps[i].Build.RawEnv = nil
374+
375+
// Recursively process any nested steps
376+
if len(steps[i].Steps) > 0 {
377+
processNestedStepsEnv(steps[i].Steps, env)
378+
}
379+
}
380+
}
381+
349382
// appends top level env to Step.Env and Step.Build.Env
350383
func appendEnv(watch *WatchConfig, env map[string]string) {
351384
watch.Step.Env, _ = parseEnv(watch.Step.RawEnv)
@@ -372,6 +405,10 @@ func appendEnv(watch *WatchConfig, env map[string]string) {
372405

373406
watch.Step.RawEnv = nil
374407
watch.Step.Build.RawEnv = nil
408+
// Process nested steps' environment variables
409+
if len(watch.Step.Steps) > 0 {
410+
processNestedStepsEnv(watch.Step.Steps, env)
411+
}
375412
watch.RawPath = nil
376413
watch.RawSkipPath = nil
377414
}

‎plugin_test.go‎

Lines changed: 82 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -292,8 +292,22 @@ func TestPluginShouldUnmarshallCorrectly(t *testing.T) {
292292
Step: Step{
293293
Group: "my group",
294294
Steps: []Step{
295-
{Command: "echo hello-group from first step"},
296-
{Command: "echo hello-group from second step"},
295+
{
296+
Command: "echo hello-group from first step",
297+
Env: map[string]string{
298+
"env1": "env-1",
299+
"env2": "env-2",
300+
"env3": "env-3",
301+
},
302+
},
303+
{
304+
Command: "echo hello-group from second step",
305+
Env: map[string]string{
306+
"env1": "env-1",
307+
"env2": "env-2",
308+
"env3": "env-3",
309+
},
310+
},
297311
},
298312
},
299313
},
@@ -863,3 +877,69 @@ func TestPluginShouldPreserveStepCondition(t *testing.T) {
863877
t.Fatalf("plugin diff (-want +got):\n%s", diff)
864878
}
865879
}
880+
881+
func TestPluginShouldClearRawEnvFromNestedSteps(t *testing.T) {
882+
param := `[{
883+
"github.com/buildkite-plugins/monorepo-diff-buildkite-plugin#commit": {
884+
"env": [
885+
"PLUGIN_ENV=plugin-value"
886+
],
887+
"watch": [
888+
{
889+
"path": "yarn.lock",
890+
"config": {
891+
"group": "test-group",
892+
"steps": [
893+
{
894+
"trigger": "test-pipeline",
895+
"label": "PR build",
896+
"build": {
897+
"commit": "abc123",
898+
"branch": "feature-branch",
899+
"env": [
900+
"BUNDLE_BUILD=pr"
901+
]
902+
}
903+
},
904+
{
905+
"trigger": "test-pipeline",
906+
"label": "Master build",
907+
"build": {
908+
"branch": "master",
909+
"env": [
910+
"BUNDLE_BUILD=master"
911+
]
912+
}
913+
}
914+
]
915+
}
916+
}
917+
]
918+
}
919+
}]`
920+
921+
got, err := initializePlugin(param)
922+
assert.NoError(t, err)
923+
924+
// Verify the structure is correct
925+
assert.Equal(t, 1, len(got.Watch))
926+
assert.Equal(t, "test-group", got.Watch[0].Step.Group)
927+
assert.Equal(t, 2, len(got.Watch[0].Step.Steps))
928+
929+
// Verify nested steps have correct env values
930+
firstStep := got.Watch[0].Step.Steps[0]
931+
assert.Equal(t, "test-pipeline", firstStep.Trigger)
932+
assert.Equal(t, "pr", firstStep.Build.Env["BUNDLE_BUILD"])
933+
assert.Equal(t, "plugin-value", firstStep.Build.Env["PLUGIN_ENV"])
934+
935+
secondStep := got.Watch[0].Step.Steps[1]
936+
assert.Equal(t, "test-pipeline", secondStep.Trigger)
937+
assert.Equal(t, "master", secondStep.Build.Env["BUNDLE_BUILD"])
938+
assert.Equal(t, "plugin-value", secondStep.Build.Env["PLUGIN_ENV"])
939+
940+
// Most importantly: verify RawEnv fields are cleared
941+
assert.Nil(t, firstStep.RawEnv, "First nested step RawEnv should be nil")
942+
assert.Nil(t, firstStep.Build.RawEnv, "First nested step Build.RawEnv should be nil")
943+
assert.Nil(t, secondStep.RawEnv, "Second nested step RawEnv should be nil")
944+
assert.Nil(t, secondStep.Build.RawEnv, "Second nested step Build.RawEnv should be nil")
945+
}

0 commit comments

Comments
 (0)