From 57f9373d5e44130cb3c82d37685681797e5156da Mon Sep 17 00:00:00 2001 From: Louis Garman Date: Sun, 8 Sep 2024 18:05:01 +0100 Subject: [PATCH] refactor: separate out task group constructor --- .../task/dependency_graph_builder_test.go | 89 +++++++++++++++++++ internal/task/group.go | 68 ++++++++++++++ internal/task/group_builder_test.go | 89 ------------------- internal/task/service.go | 66 +------------- 4 files changed, 160 insertions(+), 152 deletions(-) create mode 100644 internal/task/dependency_graph_builder_test.go delete mode 100644 internal/task/group_builder_test.go diff --git a/internal/task/dependency_graph_builder_test.go b/internal/task/dependency_graph_builder_test.go new file mode 100644 index 0000000..372b400 --- /dev/null +++ b/internal/task/dependency_graph_builder_test.go @@ -0,0 +1,89 @@ +package task + +import ( + "testing" + + "github.com/leg100/pug/internal/resource" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type fakeTaskCreator struct{} + +func (f *fakeTaskCreator) Create(spec Spec) (*Task, error) { + return (&factory{}).newTask(spec) +} + +func TestNewGroupWithDependencies(t *testing.T) { + vpcID := resource.NewID(resource.Module) + mysqlID := resource.NewID(resource.Module) + redisID := resource.NewID(resource.Module) + backendID := resource.NewID(resource.Module) + frontendID := resource.NewID(resource.Module) + mqID := resource.NewID(resource.Module) + + vpcSpec := Spec{ModuleID: &vpcID, Dependencies: &Dependencies{}} + mysqlSpec := Spec{ModuleID: &mysqlID, Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID}}} + redisSpec := Spec{ModuleID: &redisID, Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID}}} + backendSpec := Spec{ModuleID: &backendID, Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID, mysqlID, redisID}}} + frontendSpec := Spec{ModuleID: &frontendID, Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID, backendID}}} + mqSpec := Spec{ModuleID: &mqID, Dependencies: &Dependencies{}} + + t.Run("normal order", func(t *testing.T) { + got, err := createDependentTasks(&fakeTaskCreator{}, false, + vpcSpec, + mysqlSpec, + redisSpec, + backendSpec, + frontendSpec, + mqSpec, + ) + require.NoError(t, err) + + if assert.Len(t, got, 6) { + vpcTask := hasDependencies(t, got, vpcID) // 0 dependencies + mysqlTask := hasDependencies(t, got, mysqlID, vpcTask) + redisTask := hasDependencies(t, got, redisID, vpcTask) + backendTask := hasDependencies(t, got, backendID, vpcTask, mysqlTask, redisTask) + _ = hasDependencies(t, got, frontendID, vpcTask, backendTask) + _ = hasDependencies(t, got, mqID) + } + }) + + t.Run("reverse order", func(t *testing.T) { + got, err := createDependentTasks(&fakeTaskCreator{}, true, + vpcSpec, + mysqlSpec, + redisSpec, + backendSpec, + frontendSpec, + mqSpec, + ) + require.NoError(t, err) + + if assert.Len(t, got, 6) { + frontendTask := hasDependencies(t, got, frontendID) // 0 dependencies + backendTask := hasDependencies(t, got, backendID, frontendTask) + mysqlTask := hasDependencies(t, got, mysqlID, backendTask) + redisTask := hasDependencies(t, got, redisID, backendTask) + _ = hasDependencies(t, got, vpcID, mysqlTask, redisTask, backendTask, frontendTask) + _ = hasDependencies(t, got, mqID) + } + }) +} + +func hasDependencies(t *testing.T, got []*Task, want resource.ID, deps ...resource.ID) resource.ID { + for _, task := range got { + if task.ModuleID != nil && *task.ModuleID == want { + // Module matches, so now check dependencies + if assert.Len(t, task.DependsOn, len(deps)) { + for _, dep := range deps { + assert.Contains(t, task.DependsOn, dep) + } + return task.ID + } + } + } + t.Fatalf("%s not found in %v", want, got) + return resource.ID{} +} diff --git a/internal/task/group.go b/internal/task/group.go index 9f0e2ae..1da5002 100644 --- a/internal/task/group.go +++ b/internal/task/group.go @@ -1,6 +1,8 @@ package task import ( + "errors" + "fmt" "slices" "time" @@ -16,6 +18,72 @@ type Group struct { CreateErrors []error } +func newGroup(service *Service, specs ...Spec) (*Group, error) { + if len(specs) == 0 { + return nil, errors.New("no specs provided") + } + g := &Group{ + ID: resource.NewID(resource.TaskGroup), + Created: time.Now(), + } + // Validate specifications. There are some settings that are incompatible + // with one another within a task group. + var ( + respectModuleDependencies *bool + inverseDependencyOrder *bool + ) + for _, spec := range specs { + // All specs must specify Dependencies or not specify Dependencies. + deps := (spec.Dependencies != nil) + if respectModuleDependencies == nil { + respectModuleDependencies = &deps + } else if *respectModuleDependencies != deps { + return nil, fmt.Errorf("not all specs share same respect-module-dependencies setting") + } + // All specs specifying dependencies must set InverseDependencyOrder to + // the same value + inverse := (spec.Dependencies != nil && spec.Dependencies.InverseDependencyOrder) + if inverseDependencyOrder == nil { + inverseDependencyOrder = &inverse + } else if *inverseDependencyOrder != inverse { + return nil, fmt.Errorf("not all specs share same inverse-dependency-order setting") + } + } + if *respectModuleDependencies { + tasks, err := createDependentTasks(service, *inverseDependencyOrder, specs...) + if err != nil { + return nil, err + } + g.Tasks = tasks + } else { + for _, spec := range specs { + task, err := service.Create(spec) + if err != nil { + g.CreateErrors = append(g.CreateErrors, err) + continue + } + g.Tasks = append(g.Tasks, task) + } + } + if len(g.Tasks) == 0 { + return g, errors.New("all tasks failed to be created") + } + + for _, task := range g.Tasks { + if g.Command == "" { + g.Command = task.String() + } else if g.Command != task.String() { + // Detected that not all tasks have the same command, so name the + // task group to reflect that multiple commands comprise the group. + // + // TODO: make a constant + g.Command = "multi" + } + } + + return g, nil +} + func (g *Group) String() string { return g.Command } func (g *Group) IncludesTask(taskID resource.ID) bool { diff --git a/internal/task/group_builder_test.go b/internal/task/group_builder_test.go deleted file mode 100644 index 157c45e..0000000 --- a/internal/task/group_builder_test.go +++ /dev/null @@ -1,89 +0,0 @@ -package task - -import ( - "testing" - - "github.com/leg100/pug/internal/resource" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -type fakeTaskCreator struct{} - -func (f *fakeTaskCreator) Create(spec Spec) (*Task, error) { - return (&factory{}).newTask(spec) -} - -func TestNewGroupWithDependencies(t *testing.T) { - vpcID := resource.NewID(resource.Module) - mysqlID := resource.NewID(resource.Module) - redisID := resource.NewID(resource.Module) - backendID := resource.NewID(resource.Module) - frontendID := resource.NewID(resource.Module) - mqID := resource.NewID(resource.Module) - - vpcSpec := Spec{ModuleID: &vpcID, Path: "vpc", Dependencies: &Dependencies{}} - mysqlSpec := Spec{ModuleID: &mysqlID, Path: "mysql", Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID}}} - redisSpec := Spec{ModuleID: &redisID, Path: "redis", Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID}}} - backendSpec := Spec{ModuleID: &backendID, Path: "backend", Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID, mysqlID, redisID}}} - frontendSpec := Spec{ModuleID: &frontendID, Path: "frontend", Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID, backendID}}} - mqSpec := Spec{ModuleID: &mqID, Path: "mq", Dependencies: &Dependencies{}} - - t.Run("normal order", func(t *testing.T) { - got, err := createDependentTasks(&fakeTaskCreator{}, false, - vpcSpec, - mysqlSpec, - redisSpec, - backendSpec, - frontendSpec, - mqSpec, - ) - require.NoError(t, err) - - if assert.Len(t, got, 6) { - vpcTask := hasDependencies(t, got, "vpc") // 0 dependencies - mysqlTask := hasDependencies(t, got, "mysql", vpcTask) - redisTask := hasDependencies(t, got, "redis", vpcTask) - backendTask := hasDependencies(t, got, "backend", vpcTask, mysqlTask, redisTask) - _ = hasDependencies(t, got, "frontend", vpcTask, backendTask) - _ = hasDependencies(t, got, "mq") - } - }) - - t.Run("reverse order", func(t *testing.T) { - got, err := createDependentTasks(&fakeTaskCreator{}, true, - vpcSpec, - mysqlSpec, - redisSpec, - backendSpec, - frontendSpec, - mqSpec, - ) - require.NoError(t, err) - - if assert.Len(t, got, 6) { - frontendTask := hasDependencies(t, got, "frontend") // 0 dependencies - backendTask := hasDependencies(t, got, "backend", frontendTask) - mysqlTask := hasDependencies(t, got, "mysql", backendTask) - redisTask := hasDependencies(t, got, "redis", backendTask) - _ = hasDependencies(t, got, "vpc", mysqlTask, redisTask, backendTask, frontendTask) - _ = hasDependencies(t, got, "mq") - } - }) -} - -func hasDependencies(t *testing.T, got []*Task, path string, deps ...resource.ID) resource.ID { - for _, task := range got { - if task.Path == path { - // Module matches, so now check dependencies - if assert.Len(t, task.DependsOn, len(deps)) { - for _, dep := range deps { - assert.Contains(t, task.DependsOn, dep) - } - return task.ID - } - } - } - t.Fatalf("%s not found in %v", path, got) - return resource.ID{} -} diff --git a/internal/task/service.go b/internal/task/service.go index 46c5c5f..2a57a93 100644 --- a/internal/task/service.go +++ b/internal/task/service.go @@ -1,10 +1,7 @@ package task import ( - "errors" - "fmt" "slices" - "time" "github.com/leg100/pug/internal" "github.com/leg100/pug/internal/logging" @@ -97,66 +94,9 @@ func (s *Service) Create(spec Spec) (*Task, error) { // Create a task group from one or more task specs. An error is returned if zero // specs are provided, or if it fails to create at least one task. func (s *Service) CreateGroup(specs ...Spec) (*Group, error) { - if len(specs) == 0 { - return nil, errors.New("no specs provided") - } - g := &Group{ - ID: resource.NewID(resource.TaskGroup), - Created: time.Now(), - } - // Validate specifications. There are some settings that are incompatible - // with one another within a task group. - var ( - respectModuleDependencies *bool - inverseDependencyOrder *bool - ) - for _, spec := range specs { - // All specs must specify Dependencies or not specify Dependencies. - deps := (spec.Dependencies != nil) - if respectModuleDependencies == nil { - respectModuleDependencies = &deps - } else if *respectModuleDependencies != deps { - return nil, fmt.Errorf("not all specs share same respect-module-dependencies setting") - } - // All specs specifying dependencies must set InverseDependencyOrder to - // the same value - inverse := (spec.Dependencies != nil && spec.Dependencies.InverseDependencyOrder) - if inverseDependencyOrder == nil { - inverseDependencyOrder = &inverse - } else if *inverseDependencyOrder != inverse { - return nil, fmt.Errorf("not all specs share same inverse-dependency-order setting") - } - } - if *respectModuleDependencies { - tasks, err := createDependentTasks(s, *inverseDependencyOrder, specs...) - if err != nil { - return nil, err - } - g.Tasks = tasks - } else { - for _, spec := range specs { - task, err := s.Create(spec) - if err != nil { - g.CreateErrors = append(g.CreateErrors, err) - continue - } - g.Tasks = append(g.Tasks, task) - } - } - if len(g.Tasks) == 0 { - return g, errors.New("all tasks failed to be created") - } - - for _, task := range g.Tasks { - if g.Command == "" { - g.Command = task.String() - } else if g.Command != task.String() { - // Detected that not all tasks have the same command, so name the - // task group to reflect that multiple commands comprise the group. - // - // TODO: make a constant - g.Command = "multi" - } + g, err := newGroup(s, specs...) + if err != nil { + return nil, err } s.logger.Debug("created task group", "group", g)