Skip to content

Commit

Permalink
Enhance blueprint validator to check for duplicate phase names (#1468)
Browse files Browse the repository at this point in the history 
* Validate blueprint phase names to be unique

* Improve error message

* Refactor/correct phase name tests

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
@viveksinghggits @mergify
viveksinghggits and mergify[bot] committed Jun 11, 2022
1 parent 92dea34 commit 57009e2
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 18 deletions.
18 changes: 18 additions & 0 deletions pkg/blueprint/validate/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,23 @@ func Do(bp *crv1alpha1.Blueprint, funcVersion string) error {
}
}

return validatePhaseNames(bp)
}

func validatePhaseNames(bp *crv1alpha1.Blueprint) error {
phasesCount := make(map[string]int)
for _, action := range bp.Actions {
allPhases := action.Phases
if action.DeferPhase != nil {
allPhases = append(action.Phases, *action.DeferPhase)
}

for _, phase := range allPhases {
if val := phasesCount[phase.Name]; val >= 1 {
return fmt.Errorf("%s: Duplicated phase name is not allowed. Violating phase '%s'", BPValidationErr, phase.Name)
}
phasesCount[phase.Name] = 1
}
}
return nil
}
123 changes: 105 additions & 18 deletions pkg/blueprint/validate/validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ import (
func Test(t *testing.T) { TestingT(t) }

type BlueprintTest struct {
phases []crv1alpha1.BlueprintPhase
err Checker
errContains string
deferPhase *crv1alpha1.BlueprintPhase
backupPhases []crv1alpha1.BlueprintPhase
err Checker
errContains string
deferPhase *crv1alpha1.BlueprintPhase
restorePhases []crv1alpha1.BlueprintPhase
}

const (
Expand All @@ -46,7 +47,7 @@ var _ = Suite(&ValidateBlueprint{})
func (v *ValidateBlueprint) TestValidate(c *C) {
for _, tc := range []BlueprintTest{
{
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "KubeTask",
Name: "00",
Expand Down Expand Up @@ -76,7 +77,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
err: NotNil,
},
{
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "KubeTask",
Name: "10",
Expand All @@ -99,7 +100,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
},
{
// function name is incorrect
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "KubeTasks",
Name: "20",
Expand All @@ -122,7 +123,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
err: NotNil,
},
{
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "PrepareData",
Name: "30",
Expand All @@ -136,7 +137,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
err: IsNil,
},
{
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "PrepareData",
Name: "40",
Expand All @@ -150,7 +151,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
err: NotNil,
},
{
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "PrepareData",
Name: "50",
Expand All @@ -172,7 +173,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
},
},
{
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "PrepareData",
Name: "60",
Expand All @@ -195,7 +196,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
},
},
{
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "PrepareData",
Name: "70",
Expand All @@ -219,7 +220,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
},
} {
bp := blueprint()
bp.Actions["backup"].Phases = tc.phases
bp.Actions["backup"].Phases = tc.backupPhases
if tc.deferPhase != nil {
bp.Actions["backup"].DeferPhase = tc.deferPhase
}
Expand All @@ -234,7 +235,7 @@ func (v *ValidateBlueprint) TestValidate(c *C) {
func (v *ValidateBlueprint) TestValidateNonDefaultVersion(c *C) {
for _, tc := range []BlueprintTest{
{
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "NonDefaultVersionFunc",
Name: "00",
Expand All @@ -259,7 +260,7 @@ func (v *ValidateBlueprint) TestValidateNonDefaultVersion(c *C) {
{
// blueprint with one function that is registered with default version and
// one function with non default version
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "NonDefaultVersionFunc",
Name: "10",
Expand All @@ -284,7 +285,7 @@ func (v *ValidateBlueprint) TestValidateNonDefaultVersion(c *C) {
},
{
// blueprint where both the functions are registered with non default version
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "NonDefaultVersionFunc",
Name: "20",
Expand All @@ -308,7 +309,7 @@ func (v *ValidateBlueprint) TestValidateNonDefaultVersion(c *C) {
},
{
// blueprint where both the functions are registered with default version
phases: []crv1alpha1.BlueprintPhase{
backupPhases: []crv1alpha1.BlueprintPhase{
{
Func: "PrepareData",
Name: "30",
Expand All @@ -332,7 +333,7 @@ func (v *ValidateBlueprint) TestValidateNonDefaultVersion(c *C) {
},
} {
bp := blueprint()
bp.Actions["backup"].Phases = tc.phases
bp.Actions["backup"].Phases = tc.backupPhases
err := Do(bp, nonDefaultFuncVersion)
if err != nil {
c.Assert(strings.Contains(err.Error(), tc.errContains), Equals, true)
Expand All @@ -341,12 +342,98 @@ func (v *ValidateBlueprint) TestValidateNonDefaultVersion(c *C) {
}
}

func (v *ValidateBlueprint) TestValidatePhaseNames(c *C) {
for _, tc := range []BlueprintTest{
{
backupPhases: []crv1alpha1.BlueprintPhase{
{Name: "phaseone"},
{Name: "phasetwo"},
{Name: "phasethree"},
},
restorePhases: []crv1alpha1.BlueprintPhase{
{Name: "phasefour"},
{Name: "phasefive"},
},
err: IsNil,
deferPhase: &crv1alpha1.BlueprintPhase{
Name: "phasesix",
},
},
// duplicate phase names in the same action
{
backupPhases: []crv1alpha1.BlueprintPhase{
{Name: "phaseone"},
{Name: "phaseone"},
{Name: "phasethree"},
},
restorePhases: []crv1alpha1.BlueprintPhase{
{Name: "phasefour"},
{Name: "phasefive"},
},
err: NotNil,
errContains: "Duplicated phase name is not allowed. Violating phase 'phaseone'",
deferPhase: &crv1alpha1.BlueprintPhase{
Name: "phasesix",
},
},
// duplicate phase names in different actions
{
backupPhases: []crv1alpha1.BlueprintPhase{
{Name: "phaseone"},
{Name: "phasetwo"},
{Name: "phasethree"},
},
restorePhases: []crv1alpha1.BlueprintPhase{
{Name: "phaseone"},
{Name: "phasefive"},
},
err: NotNil,
errContains: "Duplicated phase name is not allowed. Violating phase 'phaseone'",
deferPhase: &crv1alpha1.BlueprintPhase{
Name: "phasesix",
},
},
// duplicate phase names in main phase and deferPhase
{
backupPhases: []crv1alpha1.BlueprintPhase{
{Name: "phaseone"},
{Name: "phasetwo"},
{Name: "phasethree"},
},
restorePhases: []crv1alpha1.BlueprintPhase{
{Name: "phasefour"},
{Name: "phasefive"},
},
err: NotNil,
errContains: "Duplicated phase name is not allowed. Violating phase 'phaseone'",
deferPhase: &crv1alpha1.BlueprintPhase{
Name: "phaseone",
},
},
} {
bp := blueprint()
bp.Actions["backup"].Phases = tc.backupPhases
bp.Actions["restore"].Phases = tc.restorePhases
if tc.deferPhase != nil {
bp.Actions["backup"].DeferPhase = tc.deferPhase
}
err := validatePhaseNames(bp)
if err != nil {
c.Assert(strings.Contains(err.Error(), tc.errContains), Equals, true)
}
c.Assert(err, tc.err)
}
}

func blueprint() *crv1alpha1.Blueprint {
return &crv1alpha1.Blueprint{
Actions: map[string]*crv1alpha1.BlueprintAction{
"backup": {
Phases: []crv1alpha1.BlueprintPhase{},
},
"restore": {
Phases: []crv1alpha1.BlueprintPhase{},
},
},
}
}
Expand Down

0 comments on commit 57009e2

Please sign in to comment.