From e02d3439dc5940daaf2d68041cb762a4ba83234d Mon Sep 17 00:00:00 2001 From: Wanxian Yang Date: Thu, 16 Jul 2020 10:49:45 -0500 Subject: [PATCH 1/9] chore: implement run task in copilot environment --- internal/pkg/task/env_runner.go | 121 +++++++++++++++++ internal/pkg/task/env_runner_test.go | 188 +++++++++++++++++++++++++++ 2 files changed, 309 insertions(+) create mode 100644 internal/pkg/task/env_runner.go create mode 100644 internal/pkg/task/env_runner_test.go diff --git a/internal/pkg/task/env_runner.go b/internal/pkg/task/env_runner.go new file mode 100644 index 00000000000..47cb431e1a5 --- /dev/null +++ b/internal/pkg/task/env_runner.go @@ -0,0 +1,121 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package task + +import ( + "errors" + "fmt" + "github.com/aws/copilot-cli/internal/pkg/aws/ec2" + "github.com/aws/copilot-cli/internal/pkg/aws/ecs" + "github.com/aws/copilot-cli/internal/pkg/deploy" +) + +const ( + clusterResourceType = "ecs:cluster" + fmtErrNoClusterFound = "no cluster found in env %s" +) + +// EnvRunner can run an Amazon ECS task in the VPC and the cluster of an environment. +type EnvRunner struct { + // Count of the tasks to be launched. + Count int + // Group Name of the tasks that use the same task definition. + GroupName string + + // App and Env in which the tasks will be launched. + App string + Env string + + // Interfaces to interact with dependencies. Must not be nil. + VPCGetter VPCGetter + ClusterGetter ResourceGetter + Starter TaskRunner +} + +// Run runs tasks in the environment of the application, and returns the task ARNs. +func (r *EnvRunner) Run() ([]string, error) { + if err := r.validateDependencies(); err != nil { + return nil, err + } + + cluster, err := r.cluster(r.App, r.Env) + if err != nil { + return nil, err + } + + filters := r.filtersForVPCFromAppEnv() + + subnets, err := r.VPCGetter.PublicSubnetIDs(filters...) + if err != nil { + return nil, fmt.Errorf("get subnet IDs from %s: %w", r.Env, err) + } + if len(subnets) == 0 { + return nil, errNoSubnetFound + } + + securityGroups, err := r.VPCGetter.SecurityGroups(filters...) + if err != nil { + return nil, fmt.Errorf("get security groups from %s: %w", r.Env, err) + } + + arns, err := r.Starter.RunTask(ecs.RunTaskInput{ + Cluster: cluster, + Count: r.Count, + Subnets: subnets, + SecurityGroups: securityGroups, + TaskFamilyName: taskFamilyName(r.GroupName), + StartedBy: startedBy, + }) + if err != nil { + return nil, fmt.Errorf("run task %s: %w", r.GroupName, err) + } + return arns, nil +} + +func (r *EnvRunner) cluster(app, env string) (string, error) { + clusters, err := r.ClusterGetter.GetResourcesByTags(clusterResourceType, map[string]string{ + deploy.AppTagKey: app, + deploy.EnvTagKey: env, + }) + + if err != nil { + return "", fmt.Errorf("get cluster by env %s: %w", env, err) + } + + if len(clusters) == 0 { + return "", fmt.Errorf(fmtErrNoClusterFound, env) + } + + // NOTE: only one cluster is associated with an application and an environment + return clusters[0].ARN, nil +} + +func (r *EnvRunner) filtersForVPCFromAppEnv() []ec2.Filter { + return []ec2.Filter{ + ec2.Filter{ + Name: ec2.TagFilterNameForEnv, + Values: []string{r.Env}, + }, + ec2.Filter{ + Name: ec2.TagFilterNameForApp, + Values: []string{r.App}, + }, + } +} + +func (r *EnvRunner) validateDependencies() error { + if r.VPCGetter == nil { + return errors.New("vpc getter is not set") + } + + if r.ClusterGetter == nil { + return errors.New("cluster getter is not set") + } + + if r.Starter == nil { + return errors.New("starter is not set") + } + + return nil +} diff --git a/internal/pkg/task/env_runner_test.go b/internal/pkg/task/env_runner_test.go new file mode 100644 index 00000000000..91dd7e599cb --- /dev/null +++ b/internal/pkg/task/env_runner_test.go @@ -0,0 +1,188 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package task + +import ( + "errors" + "fmt" + "github.com/aws/copilot-cli/internal/pkg/aws/ec2" + "github.com/aws/copilot-cli/internal/pkg/aws/ecs" + "github.com/aws/copilot-cli/internal/pkg/aws/resourcegroups" + "github.com/aws/copilot-cli/internal/pkg/deploy" + "github.com/aws/copilot-cli/internal/pkg/task/mocks" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + "testing" +) + +func TestEnvRunner_Run(t *testing.T) { + inApp := "my-app" + inEnv := "my-env" + + resourceTagFiltersForCluster := map[string]string{ + deploy.AppTagKey: inApp, + deploy.EnvTagKey: inEnv, + } + filtersForVPCFromAppEnv := []ec2.Filter{ + ec2.Filter{ + Name: ec2.TagFilterNameForEnv, + Values: []string{inEnv}, + }, + ec2.Filter{ + Name: ec2.TagFilterNameForApp, + Values: []string{inApp}, + }, + } + + mockResourceGetterWithCluster := func(m *mocks.MockResourceGetter) { + m.EXPECT().GetResourcesByTags(clusterResourceType, resourceTagFiltersForCluster).Return([]*resourcegroups.Resource{ + &resourcegroups.Resource{ARN: "cluster-1"}, + }, nil) + } + mockVPCGetterAny := func(m *mocks.MockVPCGetter) { + m.EXPECT().SubnetIDs(gomock.Any()).AnyTimes() + m.EXPECT().SecurityGroups(gomock.Any()).AnyTimes() + } + mockStarterNotRun := func(m *mocks.MockTaskRunner) { + m.EXPECT().RunTask(gomock.Any()).Times(0) + } + + testCases := map[string]struct { + count int + groupName string + + mockVPCGetter func(m *mocks.MockVPCGetter) + mockResourceGetter func(m *mocks.MockResourceGetter) + mockStarter func(m *mocks.MockTaskRunner) + + wantedError error + wantedARNs []string + }{ + "failed to get cluster": { + mockResourceGetter: func(m *mocks.MockResourceGetter) { + m.EXPECT().GetResourcesByTags(clusterResourceType, resourceTagFiltersForCluster). + Return(nil, errors.New("error getting resources")) + }, + mockVPCGetter: mockVPCGetterAny, + mockStarter: mockStarterNotRun, + wantedError: fmt.Errorf("get cluster by env %s: error getting resources", inEnv), + }, + "no cluster found": { + mockResourceGetter: func(m *mocks.MockResourceGetter) { + m.EXPECT().GetResourcesByTags(clusterResourceType, resourceTagFiltersForCluster). + Return([]*resourcegroups.Resource{}, nil) + }, + mockVPCGetter: mockVPCGetterAny, + mockStarter: mockStarterNotRun, + wantedError: fmt.Errorf("no cluster found in env %s", inEnv), + }, + "failed to get subnets": { + mockResourceGetter: mockResourceGetterWithCluster, + mockVPCGetter: func(m *mocks.MockVPCGetter) { + m.EXPECT().PublicSubnetIDs(filtersForVPCFromAppEnv). + Return(nil, errors.New("error getting subnets")) + m.EXPECT().SecurityGroups(gomock.Any()).AnyTimes() + }, + mockStarter: mockStarterNotRun, + wantedError: fmt.Errorf("get subnet IDs from %s: error getting subnets", inEnv), + }, + "no subnet is found": { + mockResourceGetter: mockResourceGetterWithCluster, + mockVPCGetter: func(m *mocks.MockVPCGetter) { + m.EXPECT().PublicSubnetIDs(filtersForVPCFromAppEnv). + Return([]string{}, nil) + m.EXPECT().SecurityGroups(gomock.Any()).AnyTimes() + }, + mockStarter: mockStarterNotRun, + wantedError: errNoSubnetFound, + }, + "failed to get security groups": { + mockResourceGetter: mockResourceGetterWithCluster, + mockVPCGetter: func(m *mocks.MockVPCGetter) { + m.EXPECT().PublicSubnetIDs(gomock.Any()).Return([]string{"subnet-1"}, nil) + m.EXPECT().SecurityGroups(filtersForVPCFromAppEnv). + Return(nil, errors.New("error getting security groups")) + }, + mockStarter: mockStarterNotRun, + wantedError: fmt.Errorf("get security groups from %s: error getting security groups", inEnv), + }, + "failed to kick off task": { + count: 1, + groupName: "my-task", + + mockResourceGetter: mockResourceGetterWithCluster, + mockVPCGetter: func(m *mocks.MockVPCGetter) { + m.EXPECT().PublicSubnetIDs(filtersForVPCFromAppEnv).Return([]string{"subnet-1", "subnet-2"}, nil) + m.EXPECT().SecurityGroups(filtersForVPCFromAppEnv).Return([]string{"sg-1", "sg-2"}, nil) + }, + mockStarter: func(m *mocks.MockTaskRunner) { + m.EXPECT().RunTask(ecs.RunTaskInput{ + Cluster: "cluster-1", + Count: 1, + Subnets: []string{"subnet-1", "subnet-2"}, + SecurityGroups: []string{"sg-1", "sg-2"}, + TaskFamilyName: taskFamilyName("my-task"), + StartedBy: startedBy, + }).Return(nil, errors.New("error running task")) + }, + + wantedError: fmt.Errorf("run task %s: error running task", "my-task"), + }, + "run in env success": { + count: 1, + groupName: "my-task", + + mockResourceGetter: mockResourceGetterWithCluster, + mockVPCGetter: func(m *mocks.MockVPCGetter) { + m.EXPECT().PublicSubnetIDs(filtersForVPCFromAppEnv).Return([]string{"subnet-1", "subnet-2"}, nil) + m.EXPECT().SecurityGroups(filtersForVPCFromAppEnv).Return([]string{"sg-1", "sg-2"}, nil) + }, + mockStarter: func(m *mocks.MockTaskRunner) { + m.EXPECT().RunTask(ecs.RunTaskInput{ + Cluster: "cluster-1", + Count: 1, + Subnets: []string{"subnet-1", "subnet-2"}, + SecurityGroups: []string{"sg-1", "sg-2"}, + TaskFamilyName: taskFamilyName("my-task"), + StartedBy: startedBy, + }).Return([]string{"task-1"}, nil) + }, + wantedARNs: []string{"task-1"}, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockVPCGetter := mocks.NewMockVPCGetter(ctrl) + mockResourceGetter := mocks.NewMockResourceGetter(ctrl) + mockStarter := mocks.NewMockTaskRunner(ctrl) + + tc.mockVPCGetter(mockVPCGetter) + tc.mockResourceGetter(mockResourceGetter) + tc.mockStarter(mockStarter) + + task := &EnvRunner{ + Count: tc.count, + GroupName: tc.groupName, + + App: inApp, + Env: inEnv, + + VPCGetter: mockVPCGetter, + ClusterGetter: mockResourceGetter, + Starter: mockStarter, + } + + arns, err := task.Run() + if tc.wantedError != nil { + require.EqualError(t, tc.wantedError, err.Error()) + } else { + require.Nil(t, err) + require.Equal(t, tc.wantedARNs, arns) + } + }) + } +} From 6ff66a007938d40affaa60f406b0921d7c9d82ea Mon Sep 17 00:00:00 2001 From: Wanxian Yang Date: Thu, 16 Jul 2020 11:19:32 -0500 Subject: [PATCH 2/9] address feedback from #1145 --- internal/pkg/task/config_runner_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/internal/pkg/task/config_runner_test.go b/internal/pkg/task/config_runner_test.go index 4a6c5aaeef1..648305ab0fe 100644 --- a/internal/pkg/task/config_runner_test.go +++ b/internal/pkg/task/config_runner_test.go @@ -47,14 +47,7 @@ func TestNetworkConfigRunner_Run(t *testing.T) { m.EXPECT().DefaultCluster().Return("cluster-1", nil) }, mockStarter: func(m *mocks.MockTaskRunner) { - m.EXPECT().RunTask(ecs.RunTaskInput{ - Cluster: "cluster-1", - Count: 1, - Subnets: []string{"subnet-1", "subnet-2"}, - SecurityGroups: []string{"sg-1", "sg-2"}, - TaskFamilyName: taskFamilyName("my-task"), - StartedBy: startedBy, - }).Return(nil, errors.New("error running task")) + m.EXPECT().RunTask(gomock.Any()).Return(nil, errors.New("error running task")) }, wantedError: fmt.Errorf("run task my-task: error running task"), From 70c644705e583ad228090281aa0368014eb4cafb Mon Sep 17 00:00:00 2001 From: louise15926 Date: Thu, 16 Jul 2020 11:39:30 -0500 Subject: [PATCH 3/9] Update internal/pkg/task/env_runner.go Co-authored-by: Efe Karakus --- internal/pkg/task/env_runner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/task/env_runner.go b/internal/pkg/task/env_runner.go index 47cb431e1a5..3f01d8396ed 100644 --- a/internal/pkg/task/env_runner.go +++ b/internal/pkg/task/env_runner.go @@ -48,7 +48,7 @@ func (r *EnvRunner) Run() ([]string, error) { subnets, err := r.VPCGetter.PublicSubnetIDs(filters...) if err != nil { - return nil, fmt.Errorf("get subnet IDs from %s: %w", r.Env, err) + return nil, fmt.Errorf("get public subnet IDs from environment %s: %w", r.Env, err) } if len(subnets) == 0 { return nil, errNoSubnetFound From 7b851cdef6353b9d97abf27dfdc4c3f2efe8e0a5 Mon Sep 17 00:00:00 2001 From: louise15926 Date: Thu, 16 Jul 2020 11:39:36 -0500 Subject: [PATCH 4/9] Update internal/pkg/task/env_runner.go Co-authored-by: Efe Karakus --- internal/pkg/task/env_runner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/task/env_runner.go b/internal/pkg/task/env_runner.go index 3f01d8396ed..8eb5f4a0e11 100644 --- a/internal/pkg/task/env_runner.go +++ b/internal/pkg/task/env_runner.go @@ -56,7 +56,7 @@ func (r *EnvRunner) Run() ([]string, error) { securityGroups, err := r.VPCGetter.SecurityGroups(filters...) if err != nil { - return nil, fmt.Errorf("get security groups from %s: %w", r.Env, err) + return nil, fmt.Errorf("get security groups from environment %s: %w", r.Env, err) } arns, err := r.Starter.RunTask(ecs.RunTaskInput{ From cd416c1374c62efce7e5f725f97194b0218fe0cd Mon Sep 17 00:00:00 2001 From: Wanxian Yang Date: Thu, 16 Jul 2020 11:42:31 -0500 Subject: [PATCH 5/9] address feedback to move tagfilter constants from ec2 to task --- internal/pkg/aws/ec2/ec2.go | 9 ++------- internal/pkg/task/env_runner.go | 10 ++++++++-- internal/pkg/task/env_runner_test.go | 4 ++-- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/internal/pkg/aws/ec2/ec2.go b/internal/pkg/aws/ec2/ec2.go index 8cd7169fbfa..c3b5227167e 100644 --- a/internal/pkg/aws/ec2/ec2.go +++ b/internal/pkg/aws/ec2/ec2.go @@ -6,8 +6,6 @@ package ec2 import ( "fmt" - "github.com/aws/copilot-cli/internal/pkg/deploy" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/ec2" @@ -15,12 +13,9 @@ import ( const ( defaultForAZFilterName = "default-for-az" -) -// Names for tag filters -var ( - TagFilterNameForApp = fmt.Sprintf("tag:%s", deploy.AppTagKey) - TagFilterNameForEnv = fmt.Sprintf("tag:%s", deploy.EnvTagKey) + // TagFilterName is the filter name format for tag filters + TagFilterName = "tag:%s" ) var ( diff --git a/internal/pkg/task/env_runner.go b/internal/pkg/task/env_runner.go index 47cb431e1a5..1af87ae5ba3 100644 --- a/internal/pkg/task/env_runner.go +++ b/internal/pkg/task/env_runner.go @@ -16,6 +16,12 @@ const ( fmtErrNoClusterFound = "no cluster found in env %s" ) +// Names for tag filters +var ( + TagFilterNameForApp = fmt.Sprintf(ec2.TagFilterName, deploy.AppTagKey) + TagFilterNameForEnv = fmt.Sprintf(ec2.TagFilterName, deploy.EnvTagKey) +) + // EnvRunner can run an Amazon ECS task in the VPC and the cluster of an environment. type EnvRunner struct { // Count of the tasks to be launched. @@ -94,11 +100,11 @@ func (r *EnvRunner) cluster(app, env string) (string, error) { func (r *EnvRunner) filtersForVPCFromAppEnv() []ec2.Filter { return []ec2.Filter{ ec2.Filter{ - Name: ec2.TagFilterNameForEnv, + Name: TagFilterNameForEnv, Values: []string{r.Env}, }, ec2.Filter{ - Name: ec2.TagFilterNameForApp, + Name: TagFilterNameForApp, Values: []string{r.App}, }, } diff --git a/internal/pkg/task/env_runner_test.go b/internal/pkg/task/env_runner_test.go index 91dd7e599cb..8c86e637dca 100644 --- a/internal/pkg/task/env_runner_test.go +++ b/internal/pkg/task/env_runner_test.go @@ -26,11 +26,11 @@ func TestEnvRunner_Run(t *testing.T) { } filtersForVPCFromAppEnv := []ec2.Filter{ ec2.Filter{ - Name: ec2.TagFilterNameForEnv, + Name: TagFilterNameForEnv, Values: []string{inEnv}, }, ec2.Filter{ - Name: ec2.TagFilterNameForApp, + Name: TagFilterNameForApp, Values: []string{inApp}, }, } From fa4b86c3150206d6cfcd5411c0cedb93735b6365 Mon Sep 17 00:00:00 2001 From: Wanxian Yang Date: Thu, 16 Jul 2020 12:03:58 -0500 Subject: [PATCH 6/9] address feedback --- internal/pkg/task/config_runner.go | 5 ++--- internal/pkg/task/default_runner.go | 7 +++---- internal/pkg/task/env_runner.go | 15 +++++++-------- internal/pkg/task/env_runner_test.go | 8 ++++---- internal/pkg/task/task.go | 5 +++++ 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/internal/pkg/task/config_runner.go b/internal/pkg/task/config_runner.go index 9f2a1eb477b..332c146b750 100644 --- a/internal/pkg/task/config_runner.go +++ b/internal/pkg/task/config_runner.go @@ -4,7 +4,6 @@ package task import ( - "errors" "fmt" "github.com/aws/copilot-cli/internal/pkg/aws/ecs" @@ -54,11 +53,11 @@ func (r *NetworkConfigRunner) Run() ([]string, error) { func (r *NetworkConfigRunner) validateDependencies() error { if r.ClusterGetter == nil { - return errors.New("cluster getter is not set") + return errClusterGetterNil } if r.Starter == nil { - return errors.New("starter is not set") + return errStarterNil } return nil diff --git a/internal/pkg/task/default_runner.go b/internal/pkg/task/default_runner.go index 95b19828b3b..e134ac5ff8f 100644 --- a/internal/pkg/task/default_runner.go +++ b/internal/pkg/task/default_runner.go @@ -4,7 +4,6 @@ package task import ( - "errors" "fmt" "github.com/aws/copilot-cli/internal/pkg/aws/ec2" "github.com/aws/copilot-cli/internal/pkg/aws/ecs" @@ -58,15 +57,15 @@ func (r *DefaultVPCRunner) Run() ([]string, error) { func (r *DefaultVPCRunner) validateDependencies() error { if r.VPCGetter == nil { - return errors.New("vpc getter is not set") + return errVPCGetterNil } if r.ClusterGetter == nil { - return errors.New("cluster getter is not set") + return errClusterGetterNil } if r.Starter == nil { - return errors.New("starter is not set") + return errStarterNil } return nil diff --git a/internal/pkg/task/env_runner.go b/internal/pkg/task/env_runner.go index 3cfe669c8f4..04eaa48ee34 100644 --- a/internal/pkg/task/env_runner.go +++ b/internal/pkg/task/env_runner.go @@ -4,7 +4,6 @@ package task import ( - "errors" "fmt" "github.com/aws/copilot-cli/internal/pkg/aws/ec2" "github.com/aws/copilot-cli/internal/pkg/aws/ecs" @@ -18,8 +17,8 @@ const ( // Names for tag filters var ( - TagFilterNameForApp = fmt.Sprintf(ec2.TagFilterName, deploy.AppTagKey) - TagFilterNameForEnv = fmt.Sprintf(ec2.TagFilterName, deploy.EnvTagKey) + tagFilterNameForApp = fmt.Sprintf(ec2.TagFilterName, deploy.AppTagKey) + tagFilterNameForEnv = fmt.Sprintf(ec2.TagFilterName, deploy.EnvTagKey) ) // EnvRunner can run an Amazon ECS task in the VPC and the cluster of an environment. @@ -100,11 +99,11 @@ func (r *EnvRunner) cluster(app, env string) (string, error) { func (r *EnvRunner) filtersForVPCFromAppEnv() []ec2.Filter { return []ec2.Filter{ ec2.Filter{ - Name: TagFilterNameForEnv, + Name: tagFilterNameForEnv, Values: []string{r.Env}, }, ec2.Filter{ - Name: TagFilterNameForApp, + Name: tagFilterNameForApp, Values: []string{r.App}, }, } @@ -112,15 +111,15 @@ func (r *EnvRunner) filtersForVPCFromAppEnv() []ec2.Filter { func (r *EnvRunner) validateDependencies() error { if r.VPCGetter == nil { - return errors.New("vpc getter is not set") + return errVPCGetterNil } if r.ClusterGetter == nil { - return errors.New("cluster getter is not set") + return errClusterGetterNil } if r.Starter == nil { - return errors.New("starter is not set") + return errStarterNil } return nil diff --git a/internal/pkg/task/env_runner_test.go b/internal/pkg/task/env_runner_test.go index 8c86e637dca..7c5c943590e 100644 --- a/internal/pkg/task/env_runner_test.go +++ b/internal/pkg/task/env_runner_test.go @@ -26,11 +26,11 @@ func TestEnvRunner_Run(t *testing.T) { } filtersForVPCFromAppEnv := []ec2.Filter{ ec2.Filter{ - Name: TagFilterNameForEnv, + Name: tagFilterNameForEnv, Values: []string{inEnv}, }, ec2.Filter{ - Name: TagFilterNameForApp, + Name: tagFilterNameForApp, Values: []string{inApp}, }, } @@ -85,7 +85,7 @@ func TestEnvRunner_Run(t *testing.T) { m.EXPECT().SecurityGroups(gomock.Any()).AnyTimes() }, mockStarter: mockStarterNotRun, - wantedError: fmt.Errorf("get subnet IDs from %s: error getting subnets", inEnv), + wantedError: fmt.Errorf("get public subnet IDs from environment %s: error getting subnets", inEnv), }, "no subnet is found": { mockResourceGetter: mockResourceGetterWithCluster, @@ -105,7 +105,7 @@ func TestEnvRunner_Run(t *testing.T) { Return(nil, errors.New("error getting security groups")) }, mockStarter: mockStarterNotRun, - wantedError: fmt.Errorf("get security groups from %s: error getting security groups", inEnv), + wantedError: fmt.Errorf("get security groups from environment %s: error getting security groups", inEnv), }, "failed to kick off task": { count: 1, diff --git a/internal/pkg/task/task.go b/internal/pkg/task/task.go index 82f0f7bd068..3a6e1008cbc 100644 --- a/internal/pkg/task/task.go +++ b/internal/pkg/task/task.go @@ -40,6 +40,11 @@ const ( var ( errNoSubnetFound = errors.New("no subnets found") + + errVPCGetterNil = errors.New("vpc getter is not set") + errClusterGetterNil = errors.New("cluster getter is not set") + errStarterNil = errors.New("starter is not set") + fmtTaskFamilyName = "copilot-%s" ) From 66a6d0ea70b441390e27d902ea0dd3b6f3a48f16 Mon Sep 17 00:00:00 2001 From: Wanxian Yang Date: Thu, 16 Jul 2020 12:15:42 -0500 Subject: [PATCH 7/9] fix test --- Makefile | 1 + internal/pkg/aws/ec2/ec2_test.go | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index e06002ca420..cd96fe48068 100644 --- a/Makefile +++ b/Makefile @@ -152,6 +152,7 @@ gen-mocks: tools ${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/describe/mocks/mock_pipeline_status.go -source=./internal/pkg/describe/pipeline_status.go ${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/aws/ecr/mocks/mock_ecr.go -source=./internal/pkg/aws/ecr/ecr.go ${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/aws/ecs/mocks/mock_ecs.go -source=./internal/pkg/aws/ecs/ecs.go + ${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/aws/ec2/mocks/mock_ec2.go -source=./internal/pkg/aws/ec2/ec2.go ${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/aws/identity/mocks/mock_identity.go -source=./internal/pkg/aws/identity/identity.go ${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/aws/route53/mocks/mock_route53.go -source=./internal/pkg/aws/route53/route53.go ${GOBIN}/mockgen -package=mocks -destination=./internal/pkg/aws/secretsmanager/mocks/mock_secretsmanager.go -source=./internal/pkg/aws/secretsmanager/secretsmanager.go diff --git a/internal/pkg/aws/ec2/ec2_test.go b/internal/pkg/aws/ec2/ec2_test.go index e1470c4b350..704d2a77bde 100644 --- a/internal/pkg/aws/ec2/ec2_test.go +++ b/internal/pkg/aws/ec2/ec2_test.go @@ -9,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/copilot-cli/internal/pkg/aws/ec2/mocks" + "github.com/aws/copilot-cli/internal/pkg/deploy" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" "testing" @@ -17,11 +18,11 @@ import ( var ( inAppEnvFilters = []Filter{ Filter{ - Name: TagFilterNameForApp, + Name: fmt.Sprintf(TagFilterName, deploy.AppTagKey), Values: []string{"my-app"}, }, Filter{ - Name: TagFilterNameForEnv, + Name: fmt.Sprintf(TagFilterName, deploy.EnvTagKey), Values: []string{"my-env"}, }, } From 0984b6f5e6399edd64a91569f3dcb925eca00650 Mon Sep 17 00:00:00 2001 From: Wanxian Yang Date: Thu, 16 Jul 2020 12:55:37 -0500 Subject: [PATCH 8/9] address feedback --- internal/pkg/task/config_runner.go | 7 ++++-- internal/pkg/task/config_runner_test.go | 7 ++++-- internal/pkg/task/default_runner.go | 13 +++++++++--- internal/pkg/task/default_runner_test.go | 9 +++++--- internal/pkg/task/env_runner.go | 23 ++++++++++++++------ internal/pkg/task/env_runner_test.go | 27 ++++++++++++++++++++---- internal/pkg/task/errors.go | 15 +++++++++++++ internal/pkg/task/task.go | 2 ++ 8 files changed, 83 insertions(+), 20 deletions(-) create mode 100644 internal/pkg/task/errors.go diff --git a/internal/pkg/task/config_runner.go b/internal/pkg/task/config_runner.go index 332c146b750..f542965e4e0 100644 --- a/internal/pkg/task/config_runner.go +++ b/internal/pkg/task/config_runner.go @@ -33,7 +33,7 @@ func (r *NetworkConfigRunner) Run() ([]string, error) { cluster, err := r.ClusterGetter.DefaultCluster() if err != nil { - return nil, fmt.Errorf("get default cluster: %w", err) + return nil, fmt.Errorf(fmtErrDefaultCluster, err) } arns, err := r.Starter.RunTask(ecs.RunTaskInput{ @@ -45,7 +45,10 @@ func (r *NetworkConfigRunner) Run() ([]string, error) { StartedBy: startedBy, }) if err != nil { - return nil, fmt.Errorf("run task %s: %w", r.GroupName, err) + return nil, &errRunTask{ + groupName: r.GroupName, + parentErr: err, + } } return arns, nil diff --git a/internal/pkg/task/config_runner_test.go b/internal/pkg/task/config_runner_test.go index 648305ab0fe..ae21beaefab 100644 --- a/internal/pkg/task/config_runner_test.go +++ b/internal/pkg/task/config_runner_test.go @@ -34,7 +34,7 @@ func TestNetworkConfigRunner_Run(t *testing.T) { mockStarter: func(m *mocks.MockTaskRunner) { m.EXPECT().RunTask(gomock.Any()).Times(0) }, - wantedError: fmt.Errorf("get default cluster: error getting default cluster"), + wantedError: fmt.Errorf(fmtErrDefaultCluster, errors.New("error getting default cluster")), }, "failed to kick off task": { count: 1, @@ -50,7 +50,10 @@ func TestNetworkConfigRunner_Run(t *testing.T) { m.EXPECT().RunTask(gomock.Any()).Return(nil, errors.New("error running task")) }, - wantedError: fmt.Errorf("run task my-task: error running task"), + wantedError: &errRunTask{ + groupName: "my-task", + parentErr: errors.New("error running task"), + }, }, "successfully kick off task with both input subnets and security groups": { count: 1, diff --git a/internal/pkg/task/default_runner.go b/internal/pkg/task/default_runner.go index e134ac5ff8f..94afc1cac7d 100644 --- a/internal/pkg/task/default_runner.go +++ b/internal/pkg/task/default_runner.go @@ -9,6 +9,10 @@ import ( "github.com/aws/copilot-cli/internal/pkg/aws/ecs" ) +const ( + fmtErrDefaultSubnets = "get default subnet IDs: %w" +) + // DefaultVPCRunner can run an Amazon ECS task in the default VPC and the default cluster. type DefaultVPCRunner struct { // Count of the tasks to be launched. @@ -30,12 +34,12 @@ func (r *DefaultVPCRunner) Run() ([]string, error) { cluster, err := r.ClusterGetter.DefaultCluster() if err != nil { - return nil, fmt.Errorf("get default cluster: %w", err) + return nil, fmt.Errorf(fmtErrDefaultCluster, err) } subnets, err := r.VPCGetter.SubnetIDs(ec2.FilterForDefaultVPCSubnets) if err != nil { - return nil, fmt.Errorf("get default subnet IDs: %w", err) + return nil, fmt.Errorf(fmtErrDefaultSubnets, err) } if len(subnets) == 0 { return nil, errNoSubnetFound @@ -49,7 +53,10 @@ func (r *DefaultVPCRunner) Run() ([]string, error) { StartedBy: startedBy, }) if err != nil { - return nil, fmt.Errorf("run task %s: %w", r.GroupName, err) + return nil, &errRunTask{ + groupName: r.GroupName, + parentErr: err, + } } return arns, nil diff --git a/internal/pkg/task/default_runner_test.go b/internal/pkg/task/default_runner_test.go index b074d970c78..09c0aa59931 100644 --- a/internal/pkg/task/default_runner_test.go +++ b/internal/pkg/task/default_runner_test.go @@ -36,7 +36,7 @@ func TestDefaultVPCRunner_Run(t *testing.T) { mockStarter: func(m *mocks.MockTaskRunner) { m.EXPECT().RunTask(gomock.Any()).Times(0) }, - wantedError: fmt.Errorf("get default cluster: error getting cluster"), + wantedError: fmt.Errorf(fmtErrDefaultCluster, errors.New("error getting cluster")), }, "failed to get subnet": { mockClusterGetter: func(m *mocks.MockDefaultClusterGetter) { @@ -48,7 +48,7 @@ func TestDefaultVPCRunner_Run(t *testing.T) { mockStarter: func(m *mocks.MockTaskRunner) { m.EXPECT().RunTask(gomock.Any()).Times(0) }, - wantedError: errors.New("get default subnet IDs: error getting subnets"), + wantedError: fmt.Errorf(fmtErrDefaultSubnets, errors.New("error getting subnets")), }, "failed to kick off task": { count: 1, @@ -62,7 +62,10 @@ func TestDefaultVPCRunner_Run(t *testing.T) { mockStarter: func(m *mocks.MockTaskRunner) { m.EXPECT().RunTask(gomock.Any()).Return(nil, errors.New("error running task")) }, - wantedError: errors.New("run task my-task: error running task"), + wantedError: &errRunTask{ + groupName: "my-task", + parentErr: errors.New("error running task"), + }, }, } diff --git a/internal/pkg/task/env_runner.go b/internal/pkg/task/env_runner.go index 04eaa48ee34..7767abb575a 100644 --- a/internal/pkg/task/env_runner.go +++ b/internal/pkg/task/env_runner.go @@ -12,7 +12,12 @@ import ( const ( clusterResourceType = "ecs:cluster" - fmtErrNoClusterFound = "no cluster found in env %s" + + fmtErrClusterFromEnv = "get cluster by env %s: %w" + fmtErrNoClusterFoundFromEnv = "no cluster found in env %s" + fmtErrMoreThanOneClusterFromEnv = "more than one cluster is found in environment %s" + fmtErrPublicSubnetsFromEnv = "get public subnet IDs from environment %s: %w" + fmtErrSecurityGroupsFromEnv = "get security groups from environment %s: %w" ) // Names for tag filters @@ -53,7 +58,7 @@ func (r *EnvRunner) Run() ([]string, error) { subnets, err := r.VPCGetter.PublicSubnetIDs(filters...) if err != nil { - return nil, fmt.Errorf("get public subnet IDs from environment %s: %w", r.Env, err) + return nil, fmt.Errorf(fmtErrPublicSubnetsFromEnv, r.Env, err) } if len(subnets) == 0 { return nil, errNoSubnetFound @@ -61,7 +66,7 @@ func (r *EnvRunner) Run() ([]string, error) { securityGroups, err := r.VPCGetter.SecurityGroups(filters...) if err != nil { - return nil, fmt.Errorf("get security groups from environment %s: %w", r.Env, err) + return nil, fmt.Errorf(fmtErrSecurityGroupsFromEnv, r.Env, err) } arns, err := r.Starter.RunTask(ecs.RunTaskInput{ @@ -73,7 +78,10 @@ func (r *EnvRunner) Run() ([]string, error) { StartedBy: startedBy, }) if err != nil { - return nil, fmt.Errorf("run task %s: %w", r.GroupName, err) + return nil, &errRunTask{ + groupName: r.GroupName, + parentErr: err, + } } return arns, nil } @@ -85,14 +93,17 @@ func (r *EnvRunner) cluster(app, env string) (string, error) { }) if err != nil { - return "", fmt.Errorf("get cluster by env %s: %w", env, err) + return "", fmt.Errorf(fmtErrClusterFromEnv, env, err) } if len(clusters) == 0 { - return "", fmt.Errorf(fmtErrNoClusterFound, env) + return "", fmt.Errorf(fmtErrNoClusterFoundFromEnv, env) } // NOTE: only one cluster is associated with an application and an environment + if len(clusters) > 1 { + return "", fmt.Errorf(fmtErrMoreThanOneClusterFromEnv, r.Env) + } return clusters[0].ARN, nil } diff --git a/internal/pkg/task/env_runner_test.go b/internal/pkg/task/env_runner_test.go index 7c5c943590e..2572e051e61 100644 --- a/internal/pkg/task/env_runner_test.go +++ b/internal/pkg/task/env_runner_test.go @@ -66,7 +66,7 @@ func TestEnvRunner_Run(t *testing.T) { }, mockVPCGetter: mockVPCGetterAny, mockStarter: mockStarterNotRun, - wantedError: fmt.Errorf("get cluster by env %s: error getting resources", inEnv), + wantedError: fmt.Errorf(fmtErrClusterFromEnv, inEnv, errors.New("error getting resources")), }, "no cluster found": { mockResourceGetter: func(m *mocks.MockResourceGetter) { @@ -77,6 +77,22 @@ func TestEnvRunner_Run(t *testing.T) { mockStarter: mockStarterNotRun, wantedError: fmt.Errorf("no cluster found in env %s", inEnv), }, + "more than one cluster is found": { + mockResourceGetter: func(m *mocks.MockResourceGetter) { + m.EXPECT().GetResourcesByTags(clusterResourceType, resourceTagFiltersForCluster). + Return([]*resourcegroups.Resource{ + &resourcegroups.Resource{ + ARN: "cluster-1", + }, + &resourcegroups.Resource{ + ARN: "cluster-2", + }, + }, nil) + }, + mockVPCGetter: mockVPCGetterAny, + mockStarter: mockStarterNotRun, + wantedError: fmt.Errorf(fmtErrMoreThanOneClusterFromEnv, inEnv), + }, "failed to get subnets": { mockResourceGetter: mockResourceGetterWithCluster, mockVPCGetter: func(m *mocks.MockVPCGetter) { @@ -85,7 +101,7 @@ func TestEnvRunner_Run(t *testing.T) { m.EXPECT().SecurityGroups(gomock.Any()).AnyTimes() }, mockStarter: mockStarterNotRun, - wantedError: fmt.Errorf("get public subnet IDs from environment %s: error getting subnets", inEnv), + wantedError: fmt.Errorf(fmtErrPublicSubnetsFromEnv, inEnv, errors.New("error getting subnets")), }, "no subnet is found": { mockResourceGetter: mockResourceGetterWithCluster, @@ -105,7 +121,7 @@ func TestEnvRunner_Run(t *testing.T) { Return(nil, errors.New("error getting security groups")) }, mockStarter: mockStarterNotRun, - wantedError: fmt.Errorf("get security groups from environment %s: error getting security groups", inEnv), + wantedError: fmt.Errorf(fmtErrSecurityGroupsFromEnv, inEnv, errors.New("error getting security groups")), }, "failed to kick off task": { count: 1, @@ -127,7 +143,10 @@ func TestEnvRunner_Run(t *testing.T) { }).Return(nil, errors.New("error running task")) }, - wantedError: fmt.Errorf("run task %s: error running task", "my-task"), + wantedError: &errRunTask{ + groupName: "my-task", + parentErr: errors.New("error running task"), + }, }, "run in env success": { count: 1, diff --git a/internal/pkg/task/errors.go b/internal/pkg/task/errors.go new file mode 100644 index 00000000000..edb1972b2d1 --- /dev/null +++ b/internal/pkg/task/errors.go @@ -0,0 +1,15 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package task + +import "fmt" + +type errRunTask struct { + groupName string + parentErr error +} + +func (e *errRunTask) Error() string { + return fmt.Sprintf("run task %s: %v", e.groupName, e.parentErr) +} diff --git a/internal/pkg/task/task.go b/internal/pkg/task/task.go index 3a6e1008cbc..bd02224436b 100644 --- a/internal/pkg/task/task.go +++ b/internal/pkg/task/task.go @@ -45,6 +45,8 @@ var ( errClusterGetterNil = errors.New("cluster getter is not set") errStarterNil = errors.New("starter is not set") + fmtErrDefaultCluster = "get default cluster: %w" + fmtTaskFamilyName = "copilot-%s" ) From 2ef9f6f9f30746289f8eeef5ce4c4f033b9aeaa0 Mon Sep 17 00:00:00 2001 From: Wanxian Yang Date: Thu, 16 Jul 2020 14:08:56 -0500 Subject: [PATCH 9/9] address feedback --- internal/pkg/task/config_runner.go | 6 +++--- internal/pkg/task/config_runner_test.go | 5 +++-- internal/pkg/task/default_runner.go | 4 +++- internal/pkg/task/default_runner_test.go | 4 +++- internal/pkg/task/errors.go | 22 +++++++++++++++++++++- internal/pkg/task/task.go | 9 --------- 6 files changed, 33 insertions(+), 17 deletions(-) diff --git a/internal/pkg/task/config_runner.go b/internal/pkg/task/config_runner.go index f542965e4e0..d71433fd2fc 100644 --- a/internal/pkg/task/config_runner.go +++ b/internal/pkg/task/config_runner.go @@ -4,8 +4,6 @@ package task import ( - "fmt" - "github.com/aws/copilot-cli/internal/pkg/aws/ecs" ) @@ -33,7 +31,9 @@ func (r *NetworkConfigRunner) Run() ([]string, error) { cluster, err := r.ClusterGetter.DefaultCluster() if err != nil { - return nil, fmt.Errorf(fmtErrDefaultCluster, err) + return nil, &errGetDefaultCluster { + parentErr: err, + } } arns, err := r.Starter.RunTask(ecs.RunTaskInput{ diff --git a/internal/pkg/task/config_runner_test.go b/internal/pkg/task/config_runner_test.go index ae21beaefab..815e556e5cc 100644 --- a/internal/pkg/task/config_runner_test.go +++ b/internal/pkg/task/config_runner_test.go @@ -5,7 +5,6 @@ package task import ( "errors" - "fmt" "github.com/aws/copilot-cli/internal/pkg/aws/ecs" "github.com/aws/copilot-cli/internal/pkg/task/mocks" "github.com/golang/mock/gomock" @@ -34,7 +33,9 @@ func TestNetworkConfigRunner_Run(t *testing.T) { mockStarter: func(m *mocks.MockTaskRunner) { m.EXPECT().RunTask(gomock.Any()).Times(0) }, - wantedError: fmt.Errorf(fmtErrDefaultCluster, errors.New("error getting default cluster")), + wantedError: &errGetDefaultCluster{ + parentErr: errors.New("error getting default cluster"), + }, }, "failed to kick off task": { count: 1, diff --git a/internal/pkg/task/default_runner.go b/internal/pkg/task/default_runner.go index 94afc1cac7d..a92662a9179 100644 --- a/internal/pkg/task/default_runner.go +++ b/internal/pkg/task/default_runner.go @@ -34,7 +34,9 @@ func (r *DefaultVPCRunner) Run() ([]string, error) { cluster, err := r.ClusterGetter.DefaultCluster() if err != nil { - return nil, fmt.Errorf(fmtErrDefaultCluster, err) + return nil, &errGetDefaultCluster { + parentErr: err, + } } subnets, err := r.VPCGetter.SubnetIDs(ec2.FilterForDefaultVPCSubnets) diff --git a/internal/pkg/task/default_runner_test.go b/internal/pkg/task/default_runner_test.go index 09c0aa59931..14b9b3d6f2b 100644 --- a/internal/pkg/task/default_runner_test.go +++ b/internal/pkg/task/default_runner_test.go @@ -36,7 +36,9 @@ func TestDefaultVPCRunner_Run(t *testing.T) { mockStarter: func(m *mocks.MockTaskRunner) { m.EXPECT().RunTask(gomock.Any()).Times(0) }, - wantedError: fmt.Errorf(fmtErrDefaultCluster, errors.New("error getting cluster")), + wantedError: &errGetDefaultCluster{ + parentErr: errors.New("error getting cluster"), + }, }, "failed to get subnet": { mockClusterGetter: func(m *mocks.MockDefaultClusterGetter) { diff --git a/internal/pkg/task/errors.go b/internal/pkg/task/errors.go index edb1972b2d1..0a73f695d45 100644 --- a/internal/pkg/task/errors.go +++ b/internal/pkg/task/errors.go @@ -3,7 +3,19 @@ package task -import "fmt" +import ( + "errors" + "fmt" +) + +var ( + errNoSubnetFound = errors.New("no subnets found") + + errVPCGetterNil = errors.New("vpc getter is not set") + errClusterGetterNil = errors.New("cluster getter is not set") + errStarterNil = errors.New("starter is not set") + +) type errRunTask struct { groupName string @@ -13,3 +25,11 @@ type errRunTask struct { func (e *errRunTask) Error() string { return fmt.Sprintf("run task %s: %v", e.groupName, e.parentErr) } + +type errGetDefaultCluster struct { + parentErr error +} + +func (e *errGetDefaultCluster) Error() string { + return fmt.Sprintf("get default cluster: %v", e.parentErr) +} \ No newline at end of file diff --git a/internal/pkg/task/task.go b/internal/pkg/task/task.go index bd02224436b..680c0217b9c 100644 --- a/internal/pkg/task/task.go +++ b/internal/pkg/task/task.go @@ -5,7 +5,6 @@ package task import ( - "errors" "fmt" "github.com/aws/copilot-cli/internal/pkg/aws/ec2" "github.com/aws/copilot-cli/internal/pkg/aws/ecs" @@ -39,14 +38,6 @@ const ( ) var ( - errNoSubnetFound = errors.New("no subnets found") - - errVPCGetterNil = errors.New("vpc getter is not set") - errClusterGetterNil = errors.New("cluster getter is not set") - errStarterNil = errors.New("starter is not set") - - fmtErrDefaultCluster = "get default cluster: %w" - fmtTaskFamilyName = "copilot-%s" )