From 96c0fa9aea9a00cf9d8e38740765629228a49750 Mon Sep 17 00:00:00 2001 From: Simone Gotti Date: Sat, 8 Jun 2019 16:07:15 +0200 Subject: [PATCH] config: refactor config unmarshal * Don't use a complex UnmarshalJSON for Task but use specific UnmarshalJSON for every type that requires custom unmarshaling --- internal/config/config.go | 308 ++++++++++++++------------- internal/config/config_test.go | 36 ++-- internal/runconfig/runconfig.go | 13 +- internal/runconfig/runconfig_test.go | 21 +- 4 files changed, 199 insertions(+), 179 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 28ae524..bc1a70b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -108,11 +108,11 @@ type Task struct { WorkingDir string `json:"working_dir"` Shell string `json:"shell"` User string `json:"user"` - Steps []interface{} `json:"steps"` - Depends []*Depend `json:"depends"` + Steps Steps `json:"steps"` + Depends Depends `json:"depends"` IgnoreFailure bool `json:"ignore_failure"` Approval bool `json:"approval"` - When *types.When `json:"when"` + When *When `json:"when"` DockerRegistriesAuth map[string]*DockerRegistryAuth `json:"docker_registries_auth"` } @@ -124,22 +124,29 @@ const ( DependConditionOnSkipped DependCondition = "on_skipped" ) +type Depends []*Depend + type Depend struct { TaskName string `json:"task"` Conditions []DependCondition `json:"conditions"` } -type Step struct { +type Step interface{} + +type Steps []Step + +type BaseStep struct { Type string `json:"type"` Name string `json:"name"` + When *When `json:"when"` } type CloneStep struct { - Step `json:",inline"` + BaseStep `json:",inline"` } type RunStep struct { - Step `json:",inline"` + BaseStep `json:",inline"` Command string `json:"command"` Environment map[string]Value `json:"environment,omitempty"` WorkingDir string `json:"working_dir"` @@ -147,16 +154,26 @@ type RunStep struct { User string `json:"user"` } -type ValueType int +type SaveToWorkspaceStep struct { + BaseStep `json:",inline"` + Contents []*SaveContent `json:"contents"` +} -const ( - ValueTypeString ValueType = iota - ValueTypeFromVariable -) +type RestoreWorkspaceStep struct { + BaseStep `json:",inline"` + DestDir string `json:"dest_dir"` +} -type Value struct { - Type ValueType - Value string +type SaveCacheStep struct { + BaseStep `json:",inline"` + Key string `json:"key"` + Contents []*SaveContent `json:"contents"` +} + +type RestoreCacheStep struct { + BaseStep `json:",inline"` + Keys []string `json:"keys"` + DestDir string `json:"dest_dir"` } type SaveContent struct { @@ -165,141 +182,101 @@ type SaveContent struct { Paths []string `json:"paths"` } -type SaveToWorkspaceStep struct { - Step `json:",inline"` - Contents []*SaveContent `json:"contents"` -} - -type RestoreWorkspaceStep struct { - Step `json:",inline"` - DestDir string `json:"dest_dir"` -} - -type SaveCacheStep struct { - Step `json:",inline"` - Key string `json:"key"` - Contents []*SaveContent `json:"contents"` -} - -type RestoreCacheStep struct { - Step `json:",inline"` - Keys []string `json:"keys"` - DestDir string `json:"dest_dir"` -} - -func (t *Task) UnmarshalJSON(b []byte) error { - type when struct { - Branch interface{} `json:"branch"` - Tag interface{} `json:"tag"` - Ref interface{} `json:"ref"` - } - - type runtask struct { - Name string `json:"name"` - Runtime *Runtime `json:"runtime"` - Environment map[string]Value `json:"environment,omitempty"` - WorkingDir string `json:"working_dir"` - Shell string `json:"shell"` - User string `json:"user"` - Steps []map[string]interface{} `json:"steps"` - Depends []interface{} `json:"depends"` - IgnoreFailure bool `json:"ignore_failure"` - Approval bool `json:"approval"` - When *when `json:"when"` - DockerRegistriesAuth map[string]*DockerRegistryAuth `json:"docker_registries_auth"` - } - - var tr *runtask - - if err := json.Unmarshal(b, &tr); err != nil { +func (s *Steps) UnmarshalJSON(b []byte) error { + var stepsRaw []json.RawMessage + if err := json.Unmarshal(b, &stepsRaw); err != nil { return err } - t.Name = tr.Name - t.Runtime = tr.Runtime - t.Environment = tr.Environment - t.WorkingDir = tr.WorkingDir - t.Shell = tr.Shell - t.User = tr.User - t.IgnoreFailure = tr.IgnoreFailure - t.Approval = tr.Approval - t.DockerRegistriesAuth = tr.DockerRegistriesAuth + steps := make(Steps, len(stepsRaw)) + for i, stepRaw := range stepsRaw { + var step interface{} - steps := make([]interface{}, len(tr.Steps)) - for i, stepEntry := range tr.Steps { - if _, ok := stepEntry["type"]; ok { - // handle default step definition using format { type: "steptype", other steps fields } - stepType, ok := stepEntry["type"].(string) + var stepMap map[string]json.RawMessage + if err := json.Unmarshal(stepRaw, &stepMap); err != nil { + return err + } + // handle default step definition using format { type: "steptype", other steps fields } + if _, ok := stepMap["type"]; ok { + var stepTypeI interface{} + if err := json.Unmarshal(stepMap["type"], &stepTypeI); err != nil { + return err + } + stepType, ok := stepTypeI.(string) if !ok { return errors.Errorf("step type at index %d must be a string", i) } - o, err := json.Marshal(stepEntry) - if err != nil { - return err - } + switch stepType { case "clone": var s CloneStep + if err := json.Unmarshal(stepRaw, &s); err != nil { + return err + } s.Type = stepType - steps[i] = &s + step = &s case "run": var s RunStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s case "save_to_workspace": var s SaveToWorkspaceStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s case "restore_workspace": var s RestoreWorkspaceStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s case "save_cache": var s SaveCacheStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s case "restore_cache": var s RestoreCacheStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s default: return errors.Errorf("unknown step type: %s", stepType) } } else { // handle simpler (for yaml not for json) steps definition using format "steptype": { stepSpecification } - if len(stepEntry) > 1 { + if len(stepMap) > 1 { return errors.Errorf("wrong steps description at index %d: more than one step name per list entry", i) } - for stepType, stepSpec := range stepEntry { - o, err := json.Marshal(stepSpec) - if err != nil { + for stepType, stepSpecRaw := range stepMap { + var stepSpec interface{} + if err := json.Unmarshal(stepSpecRaw, &stepSpec); err != nil { return err } + switch stepType { case "clone": var s CloneStep + if err := json.Unmarshal(stepSpecRaw, &s); err != nil { + return err + } s.Type = stepType - steps[i] = &s + step = &s case "run": var s RunStep @@ -307,62 +284,78 @@ func (t *Task) UnmarshalJSON(b []byte) error { case string: s.Command = stepSpec.(string) default: - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepSpecRaw, &s); err != nil { return err } } s.Type = stepType - steps[i] = &s + step = &s case "save_to_workspace": var s SaveToWorkspaceStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepSpecRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s case "restore_workspace": var s RestoreWorkspaceStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepSpecRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s case "save_cache": var s SaveCacheStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepSpecRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s case "restore_cache": var s RestoreCacheStep - if err := json.Unmarshal(o, &s); err != nil { + if err := json.Unmarshal(stepSpecRaw, &s); err != nil { return err } s.Type = stepType - steps[i] = &s + step = &s default: return errors.Errorf("unknown step type: %s", stepType) } } } + + steps[i] = step } - t.Steps = steps + *s = steps - depends := make([]*Depend, len(tr.Depends)) - for i, dependEntry := range tr.Depends { + return nil +} + +func (d *Depends) UnmarshalJSON(b []byte) error { + var dependsRaw []json.RawMessage + + if err := json.Unmarshal(b, &dependsRaw); err != nil { + return err + } + + depends := make([]*Depend, len(dependsRaw)) + for i, dependRaw := range dependsRaw { + var dependi interface{} + if err := json.Unmarshal(dependRaw, &dependi); err != nil { + return err + } var depend *Depend isSimpler := false - switch de := dependEntry.(type) { + switch de := dependi.(type) { // handle simpler (for yaml) depends definition using format "taskname": case string: depend = &Depend{ - TaskName: dependEntry.(string), + TaskName: dependi.(string), } case map[string]interface{}: if len(de) == 1 { @@ -378,11 +371,7 @@ func (t *Task) UnmarshalJSON(b []byte) error { } if !isSimpler { // handle default depends definition using format "task": "taskname", conditions: [ list of conditions ] - o, err := json.Marshal(dependEntry) - if err != nil { - return err - } - if err := json.Unmarshal(o, &depend); err != nil { + if err := json.Unmarshal(dependRaw, &depend); err != nil { return err } } else { @@ -392,11 +381,7 @@ func (t *Task) UnmarshalJSON(b []byte) error { } type deplist map[string][]DependCondition var dl deplist - o, err := json.Marshal(dependEntry) - if err != nil { - return err - } - if err := json.Unmarshal(o, &dl); err != nil { + if err := json.Unmarshal(dependRaw, &dl); err != nil { return err } if len(dl) != 1 { @@ -416,40 +401,23 @@ func (t *Task) UnmarshalJSON(b []byte) error { depends[i] = depend } - t.Depends = depends - - if tr.When != nil { - w := &types.When{} - - var err error - - if tr.When.Branch != nil { - w.Branch, err = parseWhenConditions(tr.When.Branch) - if err != nil { - return err - } - } - - if tr.When.Tag != nil { - w.Tag, err = parseWhenConditions(tr.When.Tag) - if err != nil { - return err - } - } - - if tr.When.Ref != nil { - w.Ref, err = parseWhenConditions(tr.When.Ref) - if err != nil { - return err - } - } - - t.When = w - } + *d = depends return nil } +type ValueType int + +const ( + ValueTypeString ValueType = iota + ValueTypeFromVariable +) + +type Value struct { + Type ValueType + Value string +} + func (val *Value) UnmarshalJSON(b []byte) error { var ival interface{} if err := json.Unmarshal(b, &ival); err != nil { @@ -477,6 +445,46 @@ func (val *Value) UnmarshalJSON(b []byte) error { return nil } +type When types.When + +type when struct { + Branch interface{} `json:"branch"` + Tag interface{} `json:"tag"` + Ref interface{} `json:"ref"` +} + +func (w *When) UnmarshalJSON(b []byte) error { + var wi *when + if err := json.Unmarshal(b, &wi); err != nil { + return err + } + + var err error + + if wi.Branch != nil { + w.Branch, err = parseWhenConditions(wi.Branch) + if err != nil { + return err + } + } + + if wi.Tag != nil { + w.Tag, err = parseWhenConditions(wi.Tag) + if err != nil { + return err + } + } + + if wi.Ref != nil { + w.Ref, err = parseWhenConditions(wi.Ref) + if err != nil { + return err + } + } + + return nil +} + func parseWhenConditions(wi interface{}) (*types.WhenConditions, error) { w := &types.WhenConditions{} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c64c6af..8d1fc54 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -307,24 +307,24 @@ func TestParseOutput(t *testing.T) { WorkingDir: defaultWorkingDir, Shell: "", User: "", - Steps: []interface{}{ - &CloneStep{Step: Step{Type: "clone"}}, + Steps: Steps{ + &CloneStep{BaseStep: BaseStep{Type: "clone"}}, &RunStep{ - Step: Step{ + BaseStep: BaseStep{ Type: "run", Name: "command01", }, Command: "command01", }, &RunStep{ - Step: Step{ + BaseStep: BaseStep{ Type: "run", Name: "name different than command", }, Command: "command02", }, &RunStep{ - Step: Step{ + BaseStep: BaseStep{ Type: "run", Name: "command03", }, @@ -335,27 +335,27 @@ func TestParseOutput(t *testing.T) { }, }, &SaveCacheStep{ - Step: Step{Type: "save_cache"}, + BaseStep: BaseStep{Type: "save_cache"}, Key: "cache-{{ arch }}", Contents: []*SaveContent{&SaveContent{SourceDir: "/go/pkg/mod/cache", Paths: []string{"**"}}}, }, - &CloneStep{Step: Step{Type: "clone"}}, + &CloneStep{BaseStep: BaseStep{Type: "clone"}}, &RunStep{ - Step: Step{ + BaseStep: BaseStep{ Type: "run", Name: "command01", }, Command: "command01", }, &RunStep{ - Step: Step{ + BaseStep: BaseStep{ Type: "run", Name: "name different than command", }, Command: "command02", }, &RunStep{ - Step: Step{ + BaseStep: BaseStep{ Type: "run", Name: "command03", }, @@ -366,14 +366,14 @@ func TestParseOutput(t *testing.T) { }, }, &SaveCacheStep{ - Step: Step{Type: "save_cache"}, + BaseStep: BaseStep{Type: "save_cache"}, Key: "cache-{{ arch }}", Contents: []*SaveContent{&SaveContent{SourceDir: "/go/pkg/mod/cache", Paths: []string{"**"}}}, }, }, IgnoreFailure: false, Approval: false, - When: &types.When{ + When: &When{ Branch: &types.WhenConditions{ Include: []types.WhenCondition{ {Type: types.WhenConditionTypeSimple, Match: "master"}, @@ -413,8 +413,8 @@ func TestParseOutput(t *testing.T) { }, }, WorkingDir: defaultWorkingDir, - Steps: []interface{}{}, - Depends: []*Depend{}, + Steps: nil, + Depends: nil, }, &Task{ Name: "task03", @@ -428,8 +428,8 @@ func TestParseOutput(t *testing.T) { }, }, WorkingDir: defaultWorkingDir, - Steps: []interface{}{}, - Depends: []*Depend{}, + Steps: nil, + Depends: nil, }, &Task{ Name: "task04", @@ -443,8 +443,8 @@ func TestParseOutput(t *testing.T) { }, }, WorkingDir: defaultWorkingDir, - Steps: []interface{}{}, - Depends: []*Depend{}, + Steps: nil, + Depends: nil, }, }, }, diff --git a/internal/runconfig/runconfig.go b/internal/runconfig/runconfig.go index e948cf7..651d571 100644 --- a/internal/runconfig/runconfig.go +++ b/internal/runconfig/runconfig.go @@ -48,6 +48,17 @@ func genRuntime(c *config.Config, ce *config.Runtime, variables map[string]strin } } +func whenFromConfigWhen(cw *config.When) *types.When { + if cw == nil { + return nil + } + return &types.When{ + Branch: cw.Branch, + Tag: cw.Tag, + Ref: cw.Ref, + } +} + func stepFromConfigStep(csi interface{}, variables map[string]string) interface{} { switch cs := csi.(type) { case *config.CloneStep: @@ -183,7 +194,7 @@ func GenRunConfigTasks(uuid util.UUIDGenerator, c *config.Config, runName string rcts := map[string]*rstypes.RunConfigTask{} for _, ct := range cr.Tasks { - include := types.MatchWhen(ct.When, branch, tag, ref) + include := types.MatchWhen(whenFromConfigWhen(ct.When), branch, tag, ref) steps := make([]interface{}, len(ct.Steps)) for i, cpts := range ct.Steps { diff --git a/internal/runconfig/runconfig_test.go b/internal/runconfig/runconfig_test.go index 0c94697..d555dae 100644 --- a/internal/runconfig/runconfig_test.go +++ b/internal/runconfig/runconfig_test.go @@ -19,11 +19,12 @@ import ( "reflect" "testing" - "github.com/google/go-cmp/cmp" "github.com/sorintlab/agola/internal/config" rstypes "github.com/sorintlab/agola/internal/services/runservice/types" "github.com/sorintlab/agola/internal/services/types" "github.com/sorintlab/agola/internal/util" + + "github.com/google/go-cmp/cmp" errors "golang.org/x/xerrors" ) @@ -673,23 +674,23 @@ func TestGenRunConfig(t *testing.T) { WorkingDir: "", Shell: "", User: "", - Steps: []interface{}{ + Steps: config.Steps{ &config.RunStep{ - Step: config.Step{ + BaseStep: config.BaseStep{ Type: "run", Name: "command01", }, Command: "command01", }, &config.RunStep{ - Step: config.Step{ + BaseStep: config.BaseStep{ Type: "run", Name: "name different than command", }, Command: "command02", }, &config.RunStep{ - Step: config.Step{ + BaseStep: config.BaseStep{ Type: "run", Name: "command03", }, @@ -704,7 +705,7 @@ func TestGenRunConfig(t *testing.T) { Depends: []*config.Depend{}, IgnoreFailure: false, Approval: false, - When: &types.When{ + When: &config.When{ Branch: &types.WhenConditions{Include: []types.WhenCondition{{Match: "master"}}}, Tag: &types.WhenConditions{Include: []types.WhenCondition{{Match: "v1.x"}, {Match: "v2.x"}}}, Ref: &types.WhenConditions{ @@ -781,9 +782,9 @@ func TestGenRunConfig(t *testing.T) { }, }, }, - Steps: []interface{}{ + Steps: config.Steps{ &config.RunStep{ - Step: config.Step{ + BaseStep: config.BaseStep{ Type: "run", Name: "command01", }, @@ -867,9 +868,9 @@ func TestGenRunConfig(t *testing.T) { }, }, }, - Steps: []interface{}{ + Steps: config.Steps{ &config.RunStep{ - Step: config.Step{ + BaseStep: config.BaseStep{ Type: "run", Name: "command01", },