From 1cf5a48d5fa9bab680b7a4af87b0ca94d05a8376 Mon Sep 17 00:00:00 2001 From: wass3r <1301201+wass3r@users.noreply.github.com> Date: Sat, 28 Jun 2025 15:30:16 -0500 Subject: [PATCH 1/4] add failing tests --- .../stages_clone_false_clone_stage_clone_step.yml | 12 ++++++++++++ ...lone_false_clone_stage_clone_step_no_commands.yml | 10 ++++++++++ .../testdata/stages_clone_stage_clone_step.yml | 9 +++++++++ .../testdata/stages_clonez_stage_clone_step.yml | 9 +++++++++ .../stages_clonez_stage_clone_step_no_commands.yml | 7 +++++++ .../native/testdata/stages_init_stage_init_step.yml | 9 +++++++++ .../native/testdata/stages_initz_stage_init_step.yml | 9 +++++++++ .../stages_initz_stage_init_step_no_commands.yml | 7 +++++++ .../native/testdata/steps_clone_false_clone_step.yml | 10 ++++++++++ .../steps_clone_false_clone_step_no_commands.yml | 8 ++++++++ compiler/native/testdata/steps_clone_step.yml | 7 +++++++ compiler/native/testdata/steps_init_clone.yml | 12 ++++++++++++ compiler/native/testdata/steps_init_step.yml | 7 +++++++ 13 files changed, 116 insertions(+) create mode 100644 compiler/native/testdata/stages_clone_false_clone_stage_clone_step.yml create mode 100644 compiler/native/testdata/stages_clone_false_clone_stage_clone_step_no_commands.yml create mode 100644 compiler/native/testdata/stages_clone_stage_clone_step.yml create mode 100644 compiler/native/testdata/stages_clonez_stage_clone_step.yml create mode 100644 compiler/native/testdata/stages_clonez_stage_clone_step_no_commands.yml create mode 100644 compiler/native/testdata/stages_init_stage_init_step.yml create mode 100644 compiler/native/testdata/stages_initz_stage_init_step.yml create mode 100644 compiler/native/testdata/stages_initz_stage_init_step_no_commands.yml create mode 100644 compiler/native/testdata/steps_clone_false_clone_step.yml create mode 100644 compiler/native/testdata/steps_clone_false_clone_step_no_commands.yml create mode 100644 compiler/native/testdata/steps_clone_step.yml create mode 100644 compiler/native/testdata/steps_init_clone.yml create mode 100644 compiler/native/testdata/steps_init_step.yml diff --git a/compiler/native/testdata/stages_clone_false_clone_stage_clone_step.yml b/compiler/native/testdata/stages_clone_false_clone_stage_clone_step.yml new file mode 100644 index 000000000..3d4a8983c --- /dev/null +++ b/compiler/native/testdata/stages_clone_false_clone_stage_clone_step.yml @@ -0,0 +1,12 @@ +version: "1" + +metadata: + clone: false + +stages: + clone: + steps: + - name: clone + image: alpine:latest + commands: + - echo 'hello' diff --git a/compiler/native/testdata/stages_clone_false_clone_stage_clone_step_no_commands.yml b/compiler/native/testdata/stages_clone_false_clone_stage_clone_step_no_commands.yml new file mode 100644 index 000000000..ad335dd5c --- /dev/null +++ b/compiler/native/testdata/stages_clone_false_clone_stage_clone_step_no_commands.yml @@ -0,0 +1,10 @@ +version: "1" + +metadata: + clone: false + +stages: + clone: + steps: + - name: clone + image: alpine:latest diff --git a/compiler/native/testdata/stages_clone_stage_clone_step.yml b/compiler/native/testdata/stages_clone_stage_clone_step.yml new file mode 100644 index 000000000..9f2496c6d --- /dev/null +++ b/compiler/native/testdata/stages_clone_stage_clone_step.yml @@ -0,0 +1,9 @@ +version: "1" + +stages: + clone: + steps: + - name: clone + image: alpine:latest + commands: + - echo 'hello' diff --git a/compiler/native/testdata/stages_clonez_stage_clone_step.yml b/compiler/native/testdata/stages_clonez_stage_clone_step.yml new file mode 100644 index 000000000..e3d2a9219 --- /dev/null +++ b/compiler/native/testdata/stages_clonez_stage_clone_step.yml @@ -0,0 +1,9 @@ +version: "1" + +stages: + clonez: + steps: + - name: clone + image: alpine:latest + commands: + - echo 'hello' diff --git a/compiler/native/testdata/stages_clonez_stage_clone_step_no_commands.yml b/compiler/native/testdata/stages_clonez_stage_clone_step_no_commands.yml new file mode 100644 index 000000000..ebb233a96 --- /dev/null +++ b/compiler/native/testdata/stages_clonez_stage_clone_step_no_commands.yml @@ -0,0 +1,7 @@ +version: "1" + +stages: + clonez: + steps: + - name: clone + image: alpine:latest diff --git a/compiler/native/testdata/stages_init_stage_init_step.yml b/compiler/native/testdata/stages_init_stage_init_step.yml new file mode 100644 index 000000000..9e090990c --- /dev/null +++ b/compiler/native/testdata/stages_init_stage_init_step.yml @@ -0,0 +1,9 @@ +version: "1" + +stages: + init: + steps: + - name: init + image: alpine:latest + commands: + - echo 'hello' diff --git a/compiler/native/testdata/stages_initz_stage_init_step.yml b/compiler/native/testdata/stages_initz_stage_init_step.yml new file mode 100644 index 000000000..a7b1fb220 --- /dev/null +++ b/compiler/native/testdata/stages_initz_stage_init_step.yml @@ -0,0 +1,9 @@ +version: "1" + +stages: + initz: + steps: + - name: init + image: alpine:latest + commands: + - echo 'hello' diff --git a/compiler/native/testdata/stages_initz_stage_init_step_no_commands.yml b/compiler/native/testdata/stages_initz_stage_init_step_no_commands.yml new file mode 100644 index 000000000..2f438a07f --- /dev/null +++ b/compiler/native/testdata/stages_initz_stage_init_step_no_commands.yml @@ -0,0 +1,7 @@ +version: "1" + +stages: + initz: + steps: + - name: init + image: alpine:latest diff --git a/compiler/native/testdata/steps_clone_false_clone_step.yml b/compiler/native/testdata/steps_clone_false_clone_step.yml new file mode 100644 index 000000000..2f7640148 --- /dev/null +++ b/compiler/native/testdata/steps_clone_false_clone_step.yml @@ -0,0 +1,10 @@ +version: "1" + +metadata: + clone: false + +steps: + - name: clone + image: alpine:latest + commands: + - echo 'hello' diff --git a/compiler/native/testdata/steps_clone_false_clone_step_no_commands.yml b/compiler/native/testdata/steps_clone_false_clone_step_no_commands.yml new file mode 100644 index 000000000..d44309b89 --- /dev/null +++ b/compiler/native/testdata/steps_clone_false_clone_step_no_commands.yml @@ -0,0 +1,8 @@ +version: "1" + +metadata: + clone: false + +steps: + - name: clone + image: alpine:latest diff --git a/compiler/native/testdata/steps_clone_step.yml b/compiler/native/testdata/steps_clone_step.yml new file mode 100644 index 000000000..eba6d04a4 --- /dev/null +++ b/compiler/native/testdata/steps_clone_step.yml @@ -0,0 +1,7 @@ +version: "1" + +steps: + - name: clone + image: alpine:latest + commands: + - echo 'hello' diff --git a/compiler/native/testdata/steps_init_clone.yml b/compiler/native/testdata/steps_init_clone.yml new file mode 100644 index 000000000..3db9ed004 --- /dev/null +++ b/compiler/native/testdata/steps_init_clone.yml @@ -0,0 +1,12 @@ +version: "1" + +steps: + - name: init + image: alpine:latest + commands: + - echo 'hello' + + - name: clone + image: alpine:latest + commands: + - echo 'hello' \ No newline at end of file diff --git a/compiler/native/testdata/steps_init_step.yml b/compiler/native/testdata/steps_init_step.yml new file mode 100644 index 000000000..91140113b --- /dev/null +++ b/compiler/native/testdata/steps_init_step.yml @@ -0,0 +1,7 @@ +version: "1" + +steps: + - name: init + image: alpine:latest + commands: + - echo 'hello' From 980755ff3205a79efb5f89b76fd582162f3fd4f8 Mon Sep 17 00:00:00 2001 From: wass3r <1301201+wass3r@users.noreply.github.com> Date: Sat, 28 Jun 2025 15:30:44 -0500 Subject: [PATCH 2/4] update test to execute new tests --- compiler/native/compile_test.go | 115 ++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/compiler/native/compile_test.go b/compiler/native/compile_test.go index c764f95a1..e60bba124 100644 --- a/compiler/native/compile_test.go +++ b/compiler/native/compile_test.go @@ -1806,6 +1806,121 @@ func TestNative_Compile_Pipeline_Type(t *testing.T) { } } +func TestNative_Compile_NameCollisionsReserved(t *testing.T) { + // setup types + name := "foo" + author := "author" + number := int64(1) + + tests := []struct { + name string + yamlFile string + wantErr bool + }{ + // clone + { + name: "stages pipeline: stage name is clone, step name is clone", + yamlFile: "testdata/stages_clone_stage_clone_step.yml", + wantErr: true, + }, + { + name: "stages pipeline: stage name is clonez, step name is clone", + yamlFile: "testdata/stages_clonez_stage_clone_step.yml", + wantErr: false, + }, + { + name: "stages pipeline: stage name is clonez, step name is clone without commands", + yamlFile: "testdata/stages_clonez_stage_clone_step_no_commands.yml", + wantErr: true, + }, + { + name: "stages pipeline: clone false, stage name is clone, step name is clone", + yamlFile: "testdata/stages_clone_false_clone_stage_clone_step.yml", + wantErr: false, + }, + { + name: "stages pipeline: clone false, stage name is clone, step name is clone without commands", + yamlFile: "testdata/stages_clone_false_clone_stage_clone_step_no_commands.yml", + wantErr: true, + }, + { + name: "steps pipeline: step named clone", + yamlFile: "testdata/steps_clone_step.yml", + wantErr: true, + }, + { + name: "steps pipeline: clone false, step named clone", + yamlFile: "testdata/steps_clone_false_clone_step.yml", + wantErr: false, + }, + { + name: "steps pipeline: clone false, step named clone without commands", + yamlFile: "testdata/steps_clone_false_clone_step_no_commands.yml", + wantErr: true, + }, + // init + { + name: "stages pipeline: stage name is init, step name is init", + yamlFile: "testdata/stages_init_stage_init_step.yml", + wantErr: true, + }, + { + name: "stages pipeline: stage name is initz, step name is init", + yamlFile: "testdata/stages_initz_stage_init_step.yml", + wantErr: false, + }, + { + name: "stages pipeline: stage name is initz, step name is init without commands", + yamlFile: "testdata/stages_initz_stage_init_step_no_commands.yml", + wantErr: true, + }, + { + name: "steps pipeline: step named init", + yamlFile: "testdata/steps_init_step.yml", + wantErr: true, + }, + { + name: "steps pipeline: steps named init and clone", + yamlFile: "testdata/steps_init_clone.yml", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // run test + yaml, err := os.ReadFile(tt.yamlFile) + if err != nil { + t.Errorf("Reading yaml file return err: %v", err) + } + + compiler, err := FromCLICommand(context.Background(), testCommand(t, "http://foo.example.com")) + if err != nil { + t.Errorf("Creating compiler returned err: %v", err) + } + + compiler.repo = &api.Repo{Name: &author} + compiler.build = &api.Build{Author: &name, Number: &number} + + got, _, err := compiler.Compile(context.Background(), yaml) + if (err != nil) != tt.wantErr { + t.Errorf("Compile() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if tt.wantErr && got != nil { + t.Errorf("Compile() returned %v, want nil when expecting error", got) + } + + if !tt.wantErr && got == nil { + t.Errorf("Compile() returned nil, want non-nil when not expecting error") + } + }) + } +} + func TestNative_Compile_StageNameCollision(t *testing.T) { // setup types name := "foo" From 033125d225a4a58918fb5fbe02339ecfd6df0888 Mon Sep 17 00:00:00 2001 From: wass3r <1301201+wass3r@users.noreply.github.com> Date: Sat, 28 Jun 2025 15:31:25 -0500 Subject: [PATCH 3/4] fix: enhance pipeline validation --- compiler/native/validate.go | 76 ++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 10 deletions(-) diff --git a/compiler/native/validate.go b/compiler/native/validate.go index e6850f372..ab7bde5ea 100644 --- a/compiler/native/validate.go +++ b/compiler/native/validate.go @@ -16,9 +16,13 @@ import ( // ValidateYAML verifies the yaml configuration is valid. func (c *Client) ValidateYAML(p *yaml.Build) error { var result error + + // clone step/stage validation will depend on this + isCloneEnabled := p.Metadata.Clone == nil || *p.Metadata.Clone + // check a version is provided if len(p.Version) == 0 { - result = multierror.Append(result, fmt.Errorf("no \"version:\" YAML property provided")) + result = multierror.Append(result, fmt.Errorf(`no "version:" YAML property provided`)) } // check that stages or steps are provided @@ -54,13 +58,13 @@ func (c *Client) ValidateYAML(p *yaml.Build) error { } // validate the stages block provided - err = validateYAMLStages(p.Stages) + err = validateYAMLStages(p.Stages, isCloneEnabled) if err != nil { result = multierror.Append(result, err) } // validate the steps block provided - err = validateYAMLSteps(p.Steps) + err = validateYAMLSteps(p.Steps, "", isCloneEnabled) if err != nil { result = multierror.Append(result, err) } @@ -70,7 +74,7 @@ func (c *Client) ValidateYAML(p *yaml.Build) error { // validateYAMLStages is a helper function that verifies the // stages block in the yaml configuration is valid. -func validateYAMLStages(s yaml.StageSlice) error { +func validateYAMLStages(s yaml.StageSlice, isCloneEnabled bool) error { for _, stage := range s { if len(stage.Name) == 0 { return fmt.Errorf("no name provided for stage") @@ -81,7 +85,7 @@ func validateYAMLStages(s yaml.StageSlice) error { return fmt.Errorf("stage %s references itself in 'needs' declaration", stage.Name) } - err := validateYAMLSteps(stage.Steps) + err := validateYAMLSteps(stage.Steps, stage.Name, isCloneEnabled) if err != nil { return err } @@ -92,7 +96,7 @@ func validateYAMLStages(s yaml.StageSlice) error { // validateYAMLSteps is a helper function that verifies the // steps block in the yaml configuration is valid. -func validateYAMLSteps(s yaml.StepSlice) error { +func validateYAMLSteps(s yaml.StepSlice, stageName string, isCloneEnabled bool) error { for _, step := range s { if len(step.Name) == 0 { return fmt.Errorf("no name provided for step") @@ -102,14 +106,20 @@ func validateYAMLSteps(s yaml.StepSlice) error { return fmt.Errorf("no image provided for step %s", step.Name) } - if step.Name == "clone" || step.Name == "init" { + // top-level step, or init step in init stage + if (stageName == "" || stageName == initStageName) && step.Name == initStepName { + continue + } + + // default clone enabled and top-level clone step, or clone step in clone stage + if isCloneEnabled && (stageName == "" || stageName == cloneStageName) && step.Name == cloneStepName { continue } if len(step.Commands) == 0 && len(step.Environment) == 0 && len(step.Parameters) == 0 && len(step.Secrets) == 0 && len(step.Template.Name) == 0 && !step.Detach { - return fmt.Errorf("no commands, environment, parameters, secrets or template provided for step %s", step.Name) + return fmt.Errorf("no commands, environment, parameters, secrets, template, or detach provided for step %s", step.Name) } } @@ -139,8 +149,14 @@ func (c *Client) ValidatePipeline(p *pipeline.Build) error { // report count for custom report containers reportCount := 0 + // validate reserved names (clone and init) for multiple occurrences + err := validateReservedNames(p) + if err != nil { + result = multierror.Append(result, err) + } + // validate the services block provided - err := validatePipelineContainers(p.Services, &reportCount, make(map[string]string), make(map[string]bool), "") + err = validatePipelineContainers(p.Services, &reportCount, make(map[string]string), make(map[string]bool), "") if err != nil { result = multierror.Append(result, err) } @@ -160,6 +176,46 @@ func (c *Client) ValidatePipeline(p *pipeline.Build) error { return result } +// validateReservedNames ensures that reserved pipeline names "clone" and "init" are used only once. +// It checks both Steps and Stages in the pipeline Build for duplicate usage of these special names. +// The "clone" stage validation can be controlled via the pipeline metadata. +func validateReservedNames(p *pipeline.Build) error { + cloneCount := 0 + initCount := 0 + + shouldValidateClone := p.Metadata.Clone + + for _, step := range p.Steps { + if step.Name == cloneStepName && shouldValidateClone { + cloneCount++ + } + + if step.Name == initStepName { + initCount++ + } + } + + for _, stage := range p.Stages { + if stage.Name == cloneStageName && shouldValidateClone { + cloneCount++ + } + + if stage.Name == initStepName { + initCount++ + } + } + + if shouldValidateClone && cloneCount > 1 { + return fmt.Errorf("only one clone step is allowed") + } + + if initCount > 1 { + return fmt.Errorf("only one init step is allowed") + } + + return nil +} + // validatePipelineStages is a helper function that verifies the // stages block in the final pipeline configuration is valid. func validatePipelineStages(s pipeline.StageSlice) error { @@ -183,7 +239,7 @@ func validatePipelineStages(s pipeline.StageSlice) error { // and that the container names are unique. func validatePipelineContainers(s pipeline.ContainerSlice, reportCount *int, reportMap map[string]string, nameMap map[string]bool, stageName string) error { for _, ctn := range s { - if ctn.Name == "clone" || ctn.Name == "init" { + if ctn.Name == cloneStepName || ctn.Name == initStepName { continue } From 137dc6aff1794d296048d294e791d65181746da2 Mon Sep 17 00:00:00 2001 From: wass3r <1301201+wass3r@users.noreply.github.com> Date: Sun, 29 Jun 2025 23:08:25 -0500 Subject: [PATCH 4/4] better words --- compiler/native/validate.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/native/validate.go b/compiler/native/validate.go index ab7bde5ea..7dbf26c07 100644 --- a/compiler/native/validate.go +++ b/compiler/native/validate.go @@ -206,11 +206,11 @@ func validateReservedNames(p *pipeline.Build) error { } if shouldValidateClone && cloneCount > 1 { - return fmt.Errorf("only one clone step is allowed") + return fmt.Errorf("only one clone step/stage is allowed - rename duplicate clone steps/stages to avoid conflicts with the reserved 'clone' name") } if initCount > 1 { - return fmt.Errorf("only one init step is allowed") + return fmt.Errorf("only one init step/stage is allowed - rename duplicate init steps/stages to avoid conflicts with the reserved 'init' name") } return nil