From e444b271aa4bd81003067073731f0142d345ea13 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Sun, 9 Apr 2023 04:38:57 -0700 Subject: [PATCH 01/24] refactor args and implement a func for build label --- .../pkg/docker/dockerengine/dockerengine.go | 10 ++++++ .../docker/dockerengine/dockerengine_test.go | 31 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 9a3e34d5e6f..3abc615e1c9 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -317,6 +317,16 @@ func userHomeDirectory() string { return home } +// DockerBuildLabel returns the docker build label as a string if platform is not empty. +func DockerBuildLabel(platform string, args []string) string { + // If host platform is not linux/amd64, show the user how the container image is being built; if the build fails (if their docker server doesn't have multi-platform-- and therefore `--platform` capability, for instance) they may see why. + var buildLabel string + if platform != "" { + buildLabel = fmt.Sprintf("Building your container image: docker %s\n", strings.Join(args, " ")) + } + return buildLabel +} + type errEmptyImageTags struct { uri string } diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index b6d22762c5f..37c6e136093 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -9,6 +9,7 @@ import ( "fmt" osexec "os/exec" "path/filepath" + "strings" "testing" "github.com/aws/copilot-cli/internal/pkg/exec" @@ -613,3 +614,33 @@ func TestIsEcrCredentialHelperEnabled(t *testing.T) { }) } } + +func TestDockerBuildLabel(t *testing.T) { + testCases := map[string]struct { + inBuildArgs []string + inPlatform string + wanted string + }{ + "if platform is not empty": { + inBuildArgs: []string{"build", "-t", "mockURI:tag1", + filepath.FromSlash("mockPath/to"), "-f", "mockPath/to/mockDockerfile", + }, + inPlatform: "windows/x86_64", + wanted: fmt.Sprintf("Building your container image: docker %s\n", + strings.Join([]string{"build", "-t", "mockURI:tag1", + filepath.FromSlash("mockPath/to"), "-f", "mockPath/to/mockDockerfile"}, " ")), + }, + "if platform is empty": { + inBuildArgs: []string{"build", "-t", "mockURI:tag1", + filepath.FromSlash("mockPath/to"), "-f", "mockPath/to/mockDockerfile", + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + got := DockerBuildLabel(tc.inPlatform, tc.inBuildArgs) + require.Equal(t, tc.wanted, got) + + }) + } +} From bdfef7a9666483b061448626b23add2c9e76c5c1 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Mon, 10 Apr 2023 14:57:52 -0700 Subject: [PATCH 02/24] fetch build label in workload and task run --- .../pkg/cli/deploy/mocks/mock_workload.go | 17 ++++++- internal/pkg/cli/deploy/workload.go | 13 +++-- internal/pkg/cli/deploy/workload_test.go | 1 + internal/pkg/cli/interfaces.go | 3 ++ internal/pkg/cli/task_run.go | 15 +++--- internal/pkg/cli/task_run_test.go | 50 ++++++++++++------- 6 files changed, 71 insertions(+), 28 deletions(-) diff --git a/internal/pkg/cli/deploy/mocks/mock_workload.go b/internal/pkg/cli/deploy/mocks/mock_workload.go index a68f5e31483..4c87e0fa9b0 100644 --- a/internal/pkg/cli/deploy/mocks/mock_workload.go +++ b/internal/pkg/cli/deploy/mocks/mock_workload.go @@ -104,6 +104,21 @@ func (mr *MockrepositoryServiceMockRecorder) Login() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Login", reflect.TypeOf((*MockrepositoryService)(nil).Login)) } +// URI mocks base method. +func (m *MockrepositoryService) URI() (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "URI") + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// URI indicates an expected call of URI. +func (mr *MockrepositoryServiceMockRecorder) URI() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "URI", reflect.TypeOf((*MockrepositoryService)(nil).URI)) +} + // Mocktemplater is a mock of templater interface. type Mocktemplater struct { ctrl *gomock.Controller @@ -476,4 +491,4 @@ func (m *MocktimeoutError) Timeout() bool { func (mr *MocktimeoutErrorMockRecorder) Timeout() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Timeout", reflect.TypeOf((*MocktimeoutError)(nil).Timeout)) -} +} \ No newline at end of file diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 65de6a5e77e..9c25739145f 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -148,6 +148,7 @@ type workloadDeployer struct { mft interface{} rawMft []byte workspacePath string + uri string // Dependencies. fs afero.Fs @@ -230,6 +231,10 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { repoName := fmt.Sprintf("%s/%s", in.App.Name, in.Name) repository := repository.NewWithURI( ecr.New(defaultSessEnvRegion), repoName, resources.RepositoryURLs[in.Name]) + uri, err := repository.URI() + if err != nil { + return nil, fmt.Errorf("get ECR repository URI: %w", err) + } store := config.NewSSMStore(identity.New(defaultSession), ssm.New(defaultSession), aws.StringValue(defaultSession.Config.Region)) envDescriber, err := describe.NewEnvDescriber(describe.NewEnvDescriberConfig{ App: in.App.Name, @@ -258,7 +263,8 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { image: in.Image, resources: resources, workspacePath: ws.Path(), - fs: afero.NewOsFs(), + uri: uri, + fs: &afero.Afero{Fs: afero.NewOsFs()}, s3Client: s3.New(envSession), addons: addons, repository: repository, @@ -356,7 +362,7 @@ func (img ContainerImageIdentifier) Tag() string { func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) error { // If it is built from local Dockerfile, build and push to the ECR repo. - buildArgsPerContainer, err := buildArgsPerContainer(d.name, d.workspacePath, d.image, d.mft) + buildArgsPerContainer, err := buildArgsPerContainer(d.name, d.workspacePath, d.uri, d.image, d.mft) if err != nil { return err } @@ -389,7 +395,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err return nil } -func buildArgsPerContainer(name, workspacePath string, img ContainerImageIdentifier, unmarshaledManifest interface{}) (map[string]*dockerengine.BuildArguments, error) { +func buildArgsPerContainer(name, workspacePath, uri string, img ContainerImageIdentifier, unmarshaledManifest interface{}) (map[string]*dockerengine.BuildArguments, error) { type dfArgs interface { BuildArgs(rootDirectory string) (map[string]*manifest.DockerBuildArgs, error) ContainerPlatform() string @@ -421,6 +427,7 @@ func buildArgsPerContainer(name, workspacePath string, img ContainerImageIdentif } labels[labelForContainerName] = container dArgs[container] = &dockerengine.BuildArguments{ + URI: uri, Dockerfile: aws.StringValue(buildArgs.Dockerfile), Context: aws.StringValue(buildArgs.Context), Args: buildArgs.Args, diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index 57a91f071d3..dbac193d3dd 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -622,6 +622,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { GitShortCommitTag: tc.inMockGitTag, }, workspacePath: mockWorkspacePath, + uri: mockURI, mft: &mockWorkloadMft{ workloadName: mockName, fileName: tc.inEnvFile, diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index d9cc3f52941..ea4ab1228de 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -177,6 +177,9 @@ type repositoryService interface { imageBuilderPusher } +type dockerBuildArgsGenerator interface { + GenerateDockerBuildArgs(in *dockerengine.BuildArguments) ([]string, error) +} type logEventsWriter interface { WriteLogEvents(opts logging.WriteLogEventsOpts) error } diff --git a/internal/pkg/cli/task_run.go b/internal/pkg/cli/task_run.go index 9a671b454e2..28955fa8e38 100644 --- a/internal/pkg/cli/task_run.go +++ b/internal/pkg/cli/task_run.go @@ -153,12 +153,13 @@ type runTaskOpts struct { prompt prompter // Fields below are configured at runtime. - deployer taskDeployer - repository repositoryService - runner taskRunner - eventsWriter eventsWriter - defaultClusterGetter defaultClusterGetter - publicIPGetter publicIPGetter + deployer taskDeployer + repository repositoryService + runner taskRunner + eventsWriter eventsWriter + defaultClusterGetter defaultClusterGetter + publicIPGetter publicIPGetter + dockerBuildArgsGenerator dockerBuildArgsGenerator provider sessionProvider sess *session.Session @@ -226,6 +227,8 @@ func newTaskRunOpts(vars runTaskVars) (*runTaskOpts, error) { return nil } + opts.dockerBuildArgsGenerator = dockerengine.New(exec.NewCmd()) + opts.configureEventsWriter = func(tasks []*task.Task) { opts.eventsWriter = logging.NewTaskClient(opts.sess, opts.groupName, tasks) } diff --git a/internal/pkg/cli/task_run_test.go b/internal/pkg/cli/task_run_test.go index b7d91ab412e..236eed2609b 100644 --- a/internal/pkg/cli/task_run_test.go +++ b/internal/pkg/cli/task_run_test.go @@ -744,21 +744,26 @@ func TestTaskRunOpts_Ask(t *testing.T) { } type runTaskMocks struct { - deployer *mocks.MocktaskDeployer - repository *mocks.MockrepositoryService - runner *mocks.MocktaskRunner - store *mocks.Mockstore - eventsWriter *mocks.MockeventsWriter - defaultClusterGetter *mocks.MockdefaultClusterGetter - publicIPGetter *mocks.MockpublicIPGetter - provider *mocks.MocksessionProvider - uploader *mocks.Mockuploader + deployer *mocks.MocktaskDeployer + repository *mocks.MockrepositoryService + runner *mocks.MocktaskRunner + store *mocks.Mockstore + eventsWriter *mocks.MockeventsWriter + defaultClusterGetter *mocks.MockdefaultClusterGetter + publicIPGetter *mocks.MockpublicIPGetter + provider *mocks.MocksessionProvider + uploader *mocks.Mockuploader + dockerBuildArgsGenerator *mocks.MockdockerBuildArgsGenerator } func mockHasDefaultCluster(m runTaskMocks) { m.defaultClusterGetter.EXPECT().HasDefaultCluster().Return(true, nil).AnyTimes() } +func mockdockerBuildArgsGenerator(m runTaskMocks) { + m.dockerBuildArgsGenerator.EXPECT().GenerateDockerBuildArgs(gomock.Any()) +} + func mockRepositoryAnytime(m runTaskMocks) { m.repository.EXPECT().Login().AnyTimes() m.repository.EXPECT().BuildAndPush(gomock.Any()).AnyTimes() @@ -800,6 +805,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.store.EXPECT().GetEnvironment(gomock.Any(), gomock.Any()).AnyTimes() m.defaultClusterGetter.EXPECT().HasDefaultCluster().Return(true, nil) m.deployer.EXPECT().DeployTask(gomock.Any()).Return(nil).AnyTimes() + mockdockerBuildArgsGenerator(m) mockRepositoryAnytime(m) m.runner.EXPECT().Run().AnyTimes() }, @@ -816,6 +822,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.provider.EXPECT().FromRole(gomock.Any(), gomock.Any()) m.deployer.EXPECT().DeployTask(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() mockRepositoryAnytime(m) + mockdockerBuildArgsGenerator(m) m.runner.EXPECT().Run().AnyTimes() }, }, @@ -875,6 +882,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.provider.EXPECT().Default().Return(&session.Session{}, nil) m.store.EXPECT().GetEnvironment(gomock.Any(), gomock.Any()).AnyTimes() m.deployer.EXPECT().DeployTask(gomock.Any()).Return(nil).Times(2) + mockdockerBuildArgsGenerator(m) mockRepositoryAnytime(m) m.runner.EXPECT().Run().Return(nil, errors.New("error running")) mockHasDefaultCluster(m) @@ -890,6 +898,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { }, nil) m.provider.EXPECT().FromRole(gomock.Any(), gomock.Any()) m.deployer.EXPECT().DeployTask(gomock.Any(), gomock.Len(1)).AnyTimes() // NOTE: matching length because gomock is unable to match function arguments. + mockdockerBuildArgsGenerator(m) mockRepositoryAnytime(m) m.runner.EXPECT().Run().AnyTimes() m.defaultClusterGetter.EXPECT().HasDefaultCluster().Times(0) @@ -900,6 +909,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.provider.EXPECT().Default().Return(&session.Session{}, nil) m.store.EXPECT().GetEnvironment(gomock.Any(), gomock.Any()).Times(0) m.deployer.EXPECT().DeployTask(gomock.Any(), gomock.Len(0)).AnyTimes() // NOTE: matching length because gomock is unable to match function arguments. + mockdockerBuildArgsGenerator(m) mockRepositoryAnytime(m) m.runner.EXPECT().Run().AnyTimes() mockHasDefaultCluster(m) @@ -984,6 +994,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.publicIPGetter.EXPECT().PublicIP("eni-1").Return("1.2.3", nil) mockHasDefaultCluster(m) mockRepositoryAnytime(m) + mockdockerBuildArgsGenerator(m) }, }, "fail to get public ips": { @@ -999,6 +1010,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.publicIPGetter.EXPECT().PublicIP("eni-1").Return("", errors.New("some error")) mockHasDefaultCluster(m) mockRepositoryAnytime(m) + mockdockerBuildArgsGenerator(m) }, // wantedError is nil because we will just not show the IP address if we can't instead of erroring out. }, @@ -1154,15 +1166,16 @@ func TestTaskRunOpts_Execute(t *testing.T) { } mocks := runTaskMocks{ - deployer: mocks.NewMocktaskDeployer(ctrl), - repository: mocks.NewMockrepositoryService(ctrl), - runner: mocks.NewMocktaskRunner(ctrl), - store: mocks.NewMockstore(ctrl), - eventsWriter: mocks.NewMockeventsWriter(ctrl), - defaultClusterGetter: mocks.NewMockdefaultClusterGetter(ctrl), - publicIPGetter: mocks.NewMockpublicIPGetter(ctrl), - provider: mocks.NewMocksessionProvider(ctrl), - uploader: mocks.NewMockuploader(ctrl), + deployer: mocks.NewMocktaskDeployer(ctrl), + repository: mocks.NewMockrepositoryService(ctrl), + runner: mocks.NewMocktaskRunner(ctrl), + store: mocks.NewMockstore(ctrl), + eventsWriter: mocks.NewMockeventsWriter(ctrl), + defaultClusterGetter: mocks.NewMockdefaultClusterGetter(ctrl), + publicIPGetter: mocks.NewMockpublicIPGetter(ctrl), + provider: mocks.NewMocksessionProvider(ctrl), + uploader: mocks.NewMockuploader(ctrl), + dockerBuildArgsGenerator: mocks.NewMockdockerBuildArgsGenerator(ctrl), } tc.setupMocks(mocks) @@ -1187,6 +1200,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { provider: mocks.provider, fs: fs.Fs, } + opts.dockerBuildArgsGenerator = mocks.dockerBuildArgsGenerator opts.configureRuntimeOpts = func() error { opts.runner = mocks.runner opts.deployer = mocks.deployer From d3f05d8c2d20fc748c87f5bb85cb0f267cb31f99 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 11 Apr 2023 02:20:34 -0700 Subject: [PATCH 03/24] create a method GenerateDockerBuildArgs on BuildArguments remove same method on CmdClient that even reduce code changes --- internal/pkg/cli/task_run.go | 15 +++--- internal/pkg/cli/task_run_test.go | 50 +++++++------------ .../pkg/docker/dockerengine/dockerengine.go | 20 ++++---- .../docker/dockerengine/dockerengine_test.go | 2 +- 4 files changed, 35 insertions(+), 52 deletions(-) diff --git a/internal/pkg/cli/task_run.go b/internal/pkg/cli/task_run.go index 28955fa8e38..9a671b454e2 100644 --- a/internal/pkg/cli/task_run.go +++ b/internal/pkg/cli/task_run.go @@ -153,13 +153,12 @@ type runTaskOpts struct { prompt prompter // Fields below are configured at runtime. - deployer taskDeployer - repository repositoryService - runner taskRunner - eventsWriter eventsWriter - defaultClusterGetter defaultClusterGetter - publicIPGetter publicIPGetter - dockerBuildArgsGenerator dockerBuildArgsGenerator + deployer taskDeployer + repository repositoryService + runner taskRunner + eventsWriter eventsWriter + defaultClusterGetter defaultClusterGetter + publicIPGetter publicIPGetter provider sessionProvider sess *session.Session @@ -227,8 +226,6 @@ func newTaskRunOpts(vars runTaskVars) (*runTaskOpts, error) { return nil } - opts.dockerBuildArgsGenerator = dockerengine.New(exec.NewCmd()) - opts.configureEventsWriter = func(tasks []*task.Task) { opts.eventsWriter = logging.NewTaskClient(opts.sess, opts.groupName, tasks) } diff --git a/internal/pkg/cli/task_run_test.go b/internal/pkg/cli/task_run_test.go index 236eed2609b..b7d91ab412e 100644 --- a/internal/pkg/cli/task_run_test.go +++ b/internal/pkg/cli/task_run_test.go @@ -744,26 +744,21 @@ func TestTaskRunOpts_Ask(t *testing.T) { } type runTaskMocks struct { - deployer *mocks.MocktaskDeployer - repository *mocks.MockrepositoryService - runner *mocks.MocktaskRunner - store *mocks.Mockstore - eventsWriter *mocks.MockeventsWriter - defaultClusterGetter *mocks.MockdefaultClusterGetter - publicIPGetter *mocks.MockpublicIPGetter - provider *mocks.MocksessionProvider - uploader *mocks.Mockuploader - dockerBuildArgsGenerator *mocks.MockdockerBuildArgsGenerator + deployer *mocks.MocktaskDeployer + repository *mocks.MockrepositoryService + runner *mocks.MocktaskRunner + store *mocks.Mockstore + eventsWriter *mocks.MockeventsWriter + defaultClusterGetter *mocks.MockdefaultClusterGetter + publicIPGetter *mocks.MockpublicIPGetter + provider *mocks.MocksessionProvider + uploader *mocks.Mockuploader } func mockHasDefaultCluster(m runTaskMocks) { m.defaultClusterGetter.EXPECT().HasDefaultCluster().Return(true, nil).AnyTimes() } -func mockdockerBuildArgsGenerator(m runTaskMocks) { - m.dockerBuildArgsGenerator.EXPECT().GenerateDockerBuildArgs(gomock.Any()) -} - func mockRepositoryAnytime(m runTaskMocks) { m.repository.EXPECT().Login().AnyTimes() m.repository.EXPECT().BuildAndPush(gomock.Any()).AnyTimes() @@ -805,7 +800,6 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.store.EXPECT().GetEnvironment(gomock.Any(), gomock.Any()).AnyTimes() m.defaultClusterGetter.EXPECT().HasDefaultCluster().Return(true, nil) m.deployer.EXPECT().DeployTask(gomock.Any()).Return(nil).AnyTimes() - mockdockerBuildArgsGenerator(m) mockRepositoryAnytime(m) m.runner.EXPECT().Run().AnyTimes() }, @@ -822,7 +816,6 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.provider.EXPECT().FromRole(gomock.Any(), gomock.Any()) m.deployer.EXPECT().DeployTask(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() mockRepositoryAnytime(m) - mockdockerBuildArgsGenerator(m) m.runner.EXPECT().Run().AnyTimes() }, }, @@ -882,7 +875,6 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.provider.EXPECT().Default().Return(&session.Session{}, nil) m.store.EXPECT().GetEnvironment(gomock.Any(), gomock.Any()).AnyTimes() m.deployer.EXPECT().DeployTask(gomock.Any()).Return(nil).Times(2) - mockdockerBuildArgsGenerator(m) mockRepositoryAnytime(m) m.runner.EXPECT().Run().Return(nil, errors.New("error running")) mockHasDefaultCluster(m) @@ -898,7 +890,6 @@ func TestTaskRunOpts_Execute(t *testing.T) { }, nil) m.provider.EXPECT().FromRole(gomock.Any(), gomock.Any()) m.deployer.EXPECT().DeployTask(gomock.Any(), gomock.Len(1)).AnyTimes() // NOTE: matching length because gomock is unable to match function arguments. - mockdockerBuildArgsGenerator(m) mockRepositoryAnytime(m) m.runner.EXPECT().Run().AnyTimes() m.defaultClusterGetter.EXPECT().HasDefaultCluster().Times(0) @@ -909,7 +900,6 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.provider.EXPECT().Default().Return(&session.Session{}, nil) m.store.EXPECT().GetEnvironment(gomock.Any(), gomock.Any()).Times(0) m.deployer.EXPECT().DeployTask(gomock.Any(), gomock.Len(0)).AnyTimes() // NOTE: matching length because gomock is unable to match function arguments. - mockdockerBuildArgsGenerator(m) mockRepositoryAnytime(m) m.runner.EXPECT().Run().AnyTimes() mockHasDefaultCluster(m) @@ -994,7 +984,6 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.publicIPGetter.EXPECT().PublicIP("eni-1").Return("1.2.3", nil) mockHasDefaultCluster(m) mockRepositoryAnytime(m) - mockdockerBuildArgsGenerator(m) }, }, "fail to get public ips": { @@ -1010,7 +999,6 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.publicIPGetter.EXPECT().PublicIP("eni-1").Return("", errors.New("some error")) mockHasDefaultCluster(m) mockRepositoryAnytime(m) - mockdockerBuildArgsGenerator(m) }, // wantedError is nil because we will just not show the IP address if we can't instead of erroring out. }, @@ -1166,16 +1154,15 @@ func TestTaskRunOpts_Execute(t *testing.T) { } mocks := runTaskMocks{ - deployer: mocks.NewMocktaskDeployer(ctrl), - repository: mocks.NewMockrepositoryService(ctrl), - runner: mocks.NewMocktaskRunner(ctrl), - store: mocks.NewMockstore(ctrl), - eventsWriter: mocks.NewMockeventsWriter(ctrl), - defaultClusterGetter: mocks.NewMockdefaultClusterGetter(ctrl), - publicIPGetter: mocks.NewMockpublicIPGetter(ctrl), - provider: mocks.NewMocksessionProvider(ctrl), - uploader: mocks.NewMockuploader(ctrl), - dockerBuildArgsGenerator: mocks.NewMockdockerBuildArgsGenerator(ctrl), + deployer: mocks.NewMocktaskDeployer(ctrl), + repository: mocks.NewMockrepositoryService(ctrl), + runner: mocks.NewMocktaskRunner(ctrl), + store: mocks.NewMockstore(ctrl), + eventsWriter: mocks.NewMockeventsWriter(ctrl), + defaultClusterGetter: mocks.NewMockdefaultClusterGetter(ctrl), + publicIPGetter: mocks.NewMockpublicIPGetter(ctrl), + provider: mocks.NewMocksessionProvider(ctrl), + uploader: mocks.NewMockuploader(ctrl), } tc.setupMocks(mocks) @@ -1200,7 +1187,6 @@ func TestTaskRunOpts_Execute(t *testing.T) { provider: mocks.provider, fs: fs.Fs, } - opts.dockerBuildArgsGenerator = mocks.dockerBuildArgsGenerator opts.configureRuntimeOpts = func() error { opts.runner = mocks.runner opts.deployer = mocks.deployer diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 3abc615e1c9..f56d94e57fa 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -289,6 +289,16 @@ func PlatformString(os, arch string) string { return fmt.Sprintf("%s/%s", os, arch) } +// DockerBuildLabel returns the docker build label as a string if platform is not empty. +func DockerBuildLabel(platform string, args []string) string { + // If host platform is not linux/amd64, show the user how the container image is being built; if the build fails (if their docker server doesn't have multi-platform-- and therefore `--platform` capability, for instance) they may see why. + var buildLabel string + if platform != "" { + buildLabel = fmt.Sprintf("Building your container image: docker %s\n", strings.Join(args, " ")) + } + return buildLabel +} + func parseCredFromDockerConfig(config []byte) (*dockerConfig, error) { /* Sample docker config file @@ -317,16 +327,6 @@ func userHomeDirectory() string { return home } -// DockerBuildLabel returns the docker build label as a string if platform is not empty. -func DockerBuildLabel(platform string, args []string) string { - // If host platform is not linux/amd64, show the user how the container image is being built; if the build fails (if their docker server doesn't have multi-platform-- and therefore `--platform` capability, for instance) they may see why. - var buildLabel string - if platform != "" { - buildLabel = fmt.Sprintf("Building your container image: docker %s\n", strings.Join(args, " ")) - } - return buildLabel -} - type errEmptyImageTags struct { uri string } diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index 37c6e136093..593c3ebf873 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -615,7 +615,7 @@ func TestIsEcrCredentialHelperEnabled(t *testing.T) { } } -func TestDockerBuildLabel(t *testing.T) { +func Test_DockerBuildLabel(t *testing.T) { testCases := map[string]struct { inBuildArgs []string inPlatform string From 331caa07ac5b3a07e0d185ce14a1747548f3adc4 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 11 Apr 2023 10:31:59 -0700 Subject: [PATCH 04/24] remove interface to appease static check --- internal/pkg/cli/interfaces.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index ea4ab1228de..d9cc3f52941 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -177,9 +177,6 @@ type repositoryService interface { imageBuilderPusher } -type dockerBuildArgsGenerator interface { - GenerateDockerBuildArgs(in *dockerengine.BuildArguments) ([]string, error) -} type logEventsWriter interface { WriteLogEvents(opts logging.WriteLogEventsOpts) error } From b95253b4889829628b15c24bea6cb57a37a2a3df Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Thu, 13 Apr 2023 13:28:42 -0700 Subject: [PATCH 05/24] concurrent build with context cancellation --- .../pkg/cli/deploy/mocks/mock_workload.go | 12 +- internal/pkg/cli/deploy/workload.go | 109 +++++++++++++++-- internal/pkg/cli/deploy/workload_test.go | 32 +++-- internal/pkg/cli/interfaces.go | 3 +- internal/pkg/cli/mocks/mock_interfaces.go | 17 +-- internal/pkg/cli/task_run.go | 5 +- internal/pkg/cli/task_run_test.go | 18 +-- .../pkg/docker/dockerengine/dockerengine.go | 23 ++-- .../docker/dockerengine/dockerengine_test.go | 115 +++++++----------- .../docker/dockerengine/mock_dockerengine.go | 20 +++ .../pkg/repository/mocks/mock_repository.go | 18 +-- internal/pkg/repository/repository.go | 12 +- internal/pkg/repository/repository_test.go | 24 ++-- internal/pkg/term/syncbuffer/syncbuffer.go | 2 +- 14 files changed, 253 insertions(+), 157 deletions(-) diff --git a/internal/pkg/cli/deploy/mocks/mock_workload.go b/internal/pkg/cli/deploy/mocks/mock_workload.go index 4c87e0fa9b0..f5c051cb987 100644 --- a/internal/pkg/cli/deploy/mocks/mock_workload.go +++ b/internal/pkg/cli/deploy/mocks/mock_workload.go @@ -5,6 +5,8 @@ package mocks import ( + context "context" + io "io" reflect "reflect" addon "github.com/aws/copilot-cli/internal/pkg/addon" @@ -75,18 +77,18 @@ func (m *MockrepositoryService) EXPECT() *MockrepositoryServiceMockRecorder { } // BuildAndPush mocks base method. -func (m *MockrepositoryService) BuildAndPush(args *dockerengine.BuildArguments) (string, error) { +func (m *MockrepositoryService) BuildAndPush(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) (string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "BuildAndPush", args) + ret := m.ctrl.Call(m, "BuildAndPush", ctx, args, w) ret0, _ := ret[0].(string) ret1, _ := ret[1].(error) return ret0, ret1 } // BuildAndPush indicates an expected call of BuildAndPush. -func (mr *MockrepositoryServiceMockRecorder) BuildAndPush(args interface{}) *gomock.Call { +func (mr *MockrepositoryServiceMockRecorder) BuildAndPush(ctx, args, w interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BuildAndPush", reflect.TypeOf((*MockrepositoryService)(nil).BuildAndPush), args) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BuildAndPush", reflect.TypeOf((*MockrepositoryService)(nil).BuildAndPush), ctx, args, w) } // Login mocks base method. @@ -491,4 +493,4 @@ func (m *MocktimeoutError) Timeout() bool { func (mr *MocktimeoutErrorMockRecorder) Timeout() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Timeout", reflect.TypeOf((*MocktimeoutError)(nil).Timeout)) -} \ No newline at end of file +} diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 9c25739145f..515d8d82990 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -5,12 +5,15 @@ package deploy import ( "bytes" + "context" "errors" "fmt" "io" "os" "path/filepath" "strings" + "sync" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/session" @@ -35,11 +38,14 @@ import ( "github.com/aws/copilot-cli/internal/pkg/template/artifactpath" "github.com/aws/copilot-cli/internal/pkg/template/diff" "github.com/aws/copilot-cli/internal/pkg/term/color" + "github.com/aws/copilot-cli/internal/pkg/term/cursor" "github.com/aws/copilot-cli/internal/pkg/term/log" termprogress "github.com/aws/copilot-cli/internal/pkg/term/progress" + "github.com/aws/copilot-cli/internal/pkg/term/syncbuffer" "github.com/aws/copilot-cli/internal/pkg/version" "github.com/aws/copilot-cli/internal/pkg/workspace" "github.com/spf13/afero" + "golang.org/x/sync/errgroup" ) const ( @@ -56,6 +62,14 @@ const ( labelForVersion = "com.aws.copilot.image.version" labelForContainerName = "com.aws.copilot.image.container.name" ) +const ( + paddingForBuildAndPush = 5 + pollIntervalForBuildAndPush = 60 * time.Millisecond +) + +var ( + numLinesForBuildAndPush = 5 +) // ActionRecommender contains methods that output action recommendation. type ActionRecommender interface { @@ -70,7 +84,7 @@ func (noopActionRecommender) RecommendedActions() []string { type repositoryService interface { Login() (string, error) - BuildAndPush(args *dockerengine.BuildArguments) (string, error) + BuildAndPush(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) (string, error) } type templater interface { @@ -264,7 +278,7 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { resources: resources, workspacePath: ws.Path(), uri: uri, - fs: &afero.Afero{Fs: afero.NewOsFs()}, + fs: afero.NewOsFs(), s3Client: s3.New(envSession), addons: addons, repository: repository, @@ -373,24 +387,86 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err if err != nil { return fmt.Errorf("login to image repository: %w", err) } + + var mu sync.Mutex out.ImageDigests = make(map[string]ContainerImageIdentifier, len(buildArgsPerContainer)) + + // counter for indexing the labeled output buffers. + count := 0 + // An array of buffers, one for each container image build and push ouput. + labeledBuffers := make([]*syncbuffer.LabeledSyncBuffer, len(buildArgsPerContainer)) + + // create a context and an error group. + g, ctx := errgroup.WithContext(context.Background()) + + // create a new cursor and hide it. + cursor := cursor.New() + cursor.Hide() + for name, buildArgs := range buildArgsPerContainer { + // create a copy of loop variables to avoid data race. + name := name + buildArgs := buildArgs + + // create a pipe for streaming the build and push output. + pr, pw := io.Pipe() + buildArgs.URI = uri + + // generate build args slice for docker build label. buildArgsList, err := buildArgs.GenerateDockerBuildArgs(dockerengine.New(exec.NewCmd())) if err != nil { return fmt.Errorf("generate docker build args: %w", err) } - // TODO(adi) handle this log in syncBuffer's Print function - log.Infof("Building your container image: docker %s\n", strings.Join(buildArgsList, " ")) - digest, err := d.repository.BuildAndPush(buildArgs) - if err != nil { - return fmt.Errorf("build and push image: %w", err) - } - out.ImageDigests[name] = ContainerImageIdentifier{ - Digest: digest, - CustomTag: d.image.CustomTag, - GitShortCommitTag: d.image.GitShortCommitTag, + buf := syncbuffer.New() + labeledBuffer := buf.WithLabel(fmt.Sprintf("Building your container image: docker %s\n", strings.Join(buildArgsList, " "))) + labeledBuffers[count] = labeledBuffer + count++ + + g.Go(func() error { + defer pw.Close() + digest, err := d.repository.BuildAndPush(ctx, buildArgs, pw) + if err != nil { + return fmt.Errorf("build and push image: %w", err) + } + mu.Lock() + out.ImageDigests[name] = ContainerImageIdentifier{ + Digest: digest, + CustomTag: d.image.CustomTag, + GitShortCommitTag: d.image.GitShortCommitTag, + } + mu.Unlock() + return nil + }) + g.Go(func() error { + if err := buf.Copy(pr); err != nil { + return fmt.Errorf("copying build and push output for container %s: %w", name, err) + } + return nil + }) + } + if isCIEnvironment() { + numLinesForBuildAndPush = -1 + } + // create a LabeledTermPrinter for rendering build and push output. + ltp := syncbuffer.NewLabeledTermPrinter(os.Stderr, labeledBuffers, syncbuffer.WithNumLines(numLinesForBuildAndPush), syncbuffer.WithPadding(paddingForBuildAndPush)) + + g.Go(func() error { + for { + if ltp.IsDone() { + break + } + if err := ltp.Print(); err != nil { + return fmt.Errorf("printing logs of docker build and push: %w", err) + } + time.Sleep(pollIntervalForBuildAndPush) } + return nil + }) + + // wait for all goroutines to complete and return any errors. + if err := g.Wait(); err != nil { + return err } return nil } @@ -441,6 +517,15 @@ func buildArgsPerContainer(name, workspacePath, uri string, img ContainerImageId return dArgs, nil } +// isCIEnvironment checks if the current environment is a continuous integration (CI) system. +// Returns true by looking for the "CI" environment variable if it's set to "true", otherwise false. +func isCIEnvironment() bool { + if ci, _ := os.LookupEnv("CI"); ci == "true" { + return true + } + return false +} + func (d *workloadDeployer) uploadArtifactsToS3(out *UploadArtifactsOutput) error { var err error out.EnvFileARNs, err = d.pushEnvFilesToS3Bucket(&pushEnvFilesToS3BucketInput{ diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index dbac193d3dd..d84c1746d47 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -32,6 +32,7 @@ import ( "github.com/aws/copilot-cli/internal/pkg/template" "github.com/aws/copilot-cli/internal/pkg/term/color" "github.com/aws/copilot-cli/internal/pkg/term/log" + "github.com/aws/copilot-cli/internal/pkg/term/syncbuffer" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" ) @@ -184,9 +185,10 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { inMockGitTag string inDockerBuildArgs map[string]*manifest.DockerBuildArgs - mock func(t *testing.T, m *deployMocks) - mockServiceDeployer func(deployer *workloadDeployer) artifactsUploader - customResourcesFunc customResourcesFunc + mock func(t *testing.T, m *deployMocks) + mockServiceDeployer func(deployer *workloadDeployer) artifactsUploader + customResourcesFunc customResourcesFunc + mockTerminalWidthFunc func(fw syncbuffer.FileWriter) (int, error) wantAddonsURL string wantEnvFileARNs map[string]string @@ -202,9 +204,10 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { Context: aws.String("mockContext"), }, }, + // mockTerminalWidthFunc: , mock: func(t *testing.T, m *deployMocks) { m.mockRepositoryService.EXPECT().Login().Return(mockURI, nil) - m.mockRepositoryService.EXPECT().BuildAndPush(&dockerengine.BuildArguments{ + m.mockRepositoryService.EXPECT().BuildAndPush(gomock.Any(), &dockerengine.BuildArguments{ URI: mockURI, Dockerfile: "mockDockerfile", Context: "mockContext", @@ -214,7 +217,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.builder": "copilot-cli", "com.aws.copilot.image.container.name": "mockWkld", }, - }).Return("", mockError) + }, gomock.Any()).Return("", mockError) }, wantErr: fmt.Errorf("build and push image: some error"), }, @@ -229,7 +232,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { }, mock: func(t *testing.T, m *deployMocks) { m.mockRepositoryService.EXPECT().Login().Return(mockURI, nil) - m.mockRepositoryService.EXPECT().BuildAndPush(&dockerengine.BuildArguments{ + m.mockRepositoryService.EXPECT().BuildAndPush(gomock.Any(), &dockerengine.BuildArguments{ URI: mockURI, Dockerfile: "mockDockerfile", Context: "mockContext", @@ -239,7 +242,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.builder": "copilot-cli", "com.aws.copilot.image.container.name": "mockWkld", }, - }).Return("mockDigest", nil) + }, gomock.Any()).Return("mockDigest", nil) m.mockAddons = nil }, wantImages: map[string]ContainerImageIdentifier{ @@ -260,7 +263,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { }, mock: func(t *testing.T, m *deployMocks) { m.mockRepositoryService.EXPECT().Login().Return(mockURI, nil) - m.mockRepositoryService.EXPECT().BuildAndPush(&dockerengine.BuildArguments{ + m.mockRepositoryService.EXPECT().BuildAndPush(gomock.Any(), &dockerengine.BuildArguments{ URI: mockURI, Dockerfile: "mockDockerfile", Context: "mockContext", @@ -270,7 +273,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.builder": "copilot-cli", "com.aws.copilot.image.container.name": "mockWkld", }, - }).Return("mockDigest", nil) + }, gomock.Any()).Return("mockDigest", nil) m.mockAddons = nil }, wantImages: map[string]ContainerImageIdentifier{ @@ -294,7 +297,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { inMockGitTag: "gitTag", mock: func(t *testing.T, m *deployMocks) { m.mockRepositoryService.EXPECT().Login().Return(mockURI, nil) - m.mockRepositoryService.EXPECT().BuildAndPush(&dockerengine.BuildArguments{ + m.mockRepositoryService.EXPECT().BuildAndPush(gomock.Any(), &dockerengine.BuildArguments{ URI: mockURI, Dockerfile: "sidecarMockDockerfile", Context: "sidecarMockContext", @@ -304,8 +307,8 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.builder": "copilot-cli", "com.aws.copilot.image.container.name": "nginx", }, - }).Return("sidecarMockDigest1", nil) - m.mockRepositoryService.EXPECT().BuildAndPush(&dockerengine.BuildArguments{ + }, gomock.Any()).Return("sidecarMockDigest1", nil) + m.mockRepositoryService.EXPECT().BuildAndPush(gomock.Any(), &dockerengine.BuildArguments{ URI: mockURI, Dockerfile: "web/Dockerfile", Context: "Users/bowie", @@ -315,7 +318,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.builder": "copilot-cli", "com.aws.copilot.image.container.name": "logging", }, - }).Return("sidecarMockDigest2", nil) + }, gomock.Any()).Return("sidecarMockDigest2", nil) m.mockAddons = nil }, wantImages: map[string]ContainerImageIdentifier{ @@ -606,6 +609,9 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { return nil, nil } } + // mockTerminalWidth := func(fw syncbuffer.FileWriter) (int, error) { + // return 80, nil + // } wkldDeployer := &workloadDeployer{ name: mockName, diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index d9cc3f52941..5598d8e9264 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -4,6 +4,7 @@ package cli import ( + "context" "encoding" "io" @@ -165,7 +166,7 @@ type secretDeleter interface { } type imageBuilderPusher interface { - BuildAndPush(args *dockerengine.BuildArguments) (string, error) + BuildAndPush(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) (string, error) } type repositoryLogin interface { diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index ca6c034f1c4..9cbf679c46d 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -5,6 +5,7 @@ package mocks import ( + context "context" encoding "encoding" io "io" reflect "reflect" @@ -1562,18 +1563,18 @@ func (m *MockimageBuilderPusher) EXPECT() *MockimageBuilderPusherMockRecorder { } // BuildAndPush mocks base method. -func (m *MockimageBuilderPusher) BuildAndPush(args *dockerengine.BuildArguments) (string, error) { +func (m *MockimageBuilderPusher) BuildAndPush(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) (string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "BuildAndPush", args) + ret := m.ctrl.Call(m, "BuildAndPush", ctx, args, w) ret0, _ := ret[0].(string) ret1, _ := ret[1].(error) return ret0, ret1 } // BuildAndPush indicates an expected call of BuildAndPush. -func (mr *MockimageBuilderPusherMockRecorder) BuildAndPush(args interface{}) *gomock.Call { +func (mr *MockimageBuilderPusherMockRecorder) BuildAndPush(ctx, args, w interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BuildAndPush", reflect.TypeOf((*MockimageBuilderPusher)(nil).BuildAndPush), args) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BuildAndPush", reflect.TypeOf((*MockimageBuilderPusher)(nil).BuildAndPush), ctx, args, w) } // MockrepositoryLogin is a mock of repositoryLogin interface. @@ -1638,18 +1639,18 @@ func (m *MockrepositoryService) EXPECT() *MockrepositoryServiceMockRecorder { } // BuildAndPush mocks base method. -func (m *MockrepositoryService) BuildAndPush(args *dockerengine.BuildArguments) (string, error) { +func (m *MockrepositoryService) BuildAndPush(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) (string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "BuildAndPush", args) + ret := m.ctrl.Call(m, "BuildAndPush", ctx, args, w) ret0, _ := ret[0].(string) ret1, _ := ret[1].(error) return ret0, ret1 } // BuildAndPush indicates an expected call of BuildAndPush. -func (mr *MockrepositoryServiceMockRecorder) BuildAndPush(args interface{}) *gomock.Call { +func (mr *MockrepositoryServiceMockRecorder) BuildAndPush(ctx, args, w interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BuildAndPush", reflect.TypeOf((*MockrepositoryService)(nil).BuildAndPush), args) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BuildAndPush", reflect.TypeOf((*MockrepositoryService)(nil).BuildAndPush), ctx, args, w) } // Login mocks base method. diff --git a/internal/pkg/cli/task_run.go b/internal/pkg/cli/task_run.go index 9a671b454e2..87e7042333c 100644 --- a/internal/pkg/cli/task_run.go +++ b/internal/pkg/cli/task_run.go @@ -5,6 +5,7 @@ package cli import ( "bytes" + "context" "errors" "fmt" "os" @@ -972,8 +973,8 @@ func (o *runTaskOpts) buildAndPushImage(uri string) error { if err != nil { return fmt.Errorf("generate docker build args: %w", err) } - log.Infof("Building your container image: docker %s\n", strings.Join(buildArgsList, " ")) - if _, err := o.repository.BuildAndPush(buildArgs); err != nil { + log.Infof(fmt.Sprintf("Building your container image: docker %s\n", strings.Join(buildArgsList, " "))) + if _, err := o.repository.BuildAndPush(context.Background(), buildArgs, log.DiagnosticWriter); err != nil { return fmt.Errorf("build and push image: %w", err) } return nil diff --git a/internal/pkg/cli/task_run_test.go b/internal/pkg/cli/task_run_test.go index b7d91ab412e..37b47e9bce8 100644 --- a/internal/pkg/cli/task_run_test.go +++ b/internal/pkg/cli/task_run_test.go @@ -5,6 +5,7 @@ package cli import ( "bytes" + "context" "errors" "fmt" "path/filepath" @@ -761,7 +762,7 @@ func mockHasDefaultCluster(m runTaskMocks) { func mockRepositoryAnytime(m runTaskMocks) { m.repository.EXPECT().Login().AnyTimes() - m.repository.EXPECT().BuildAndPush(gomock.Any()).AnyTimes() + m.repository.EXPECT().BuildAndPush(context.Background(), gomock.Any(), gomock.Any()).AnyTimes() } func TestTaskRunOpts_Execute(t *testing.T) { @@ -770,6 +771,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { mockRepoURI = "uri/repo" tag = "tag" ) + ctx := context.Background() defaultBuildArguments := dockerengine.BuildArguments{ URI: mockRepoURI, Context: filepath.Dir(defaultDockerfilePath), @@ -858,8 +860,8 @@ func TestTaskRunOpts_Execute(t *testing.T) { Command: []string{}, EntryPoint: []string{}, }).Return(nil) - m.repository.EXPECT().Login().Return(mockRepoURI, nil) - m.repository.EXPECT().BuildAndPush(gomock.Any()) + m.repository.EXPECT().Login().Return(nil) + m.repository.EXPECT().BuildAndPush(ctx, gomock.Any(), gomock.Any()) m.deployer.EXPECT().DeployTask(&deploy.CreateTaskResourcesInput{ Name: inGroupName, Image: "uri/repo:latest", @@ -912,12 +914,12 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.store.EXPECT().GetEnvironment(gomock.Any(), gomock.Any()).AnyTimes() m.deployer.EXPECT().DeployTask(gomock.Any()).AnyTimes() m.repository.EXPECT().Login().Return(mockRepoURI, nil) - m.repository.EXPECT().BuildAndPush(gomock.Eq( + m.repository.EXPECT().BuildAndPush(ctx, gomock.Eq( &dockerengine.BuildArguments{ URI: mockRepoURI, Context: filepath.Dir(defaultDockerfilePath), Tags: []string{imageTagLatest, tag}, - }), + }), gomock.Any(), ) m.runner.EXPECT().Run().AnyTimes() mockHasDefaultCluster(m) @@ -930,12 +932,12 @@ func TestTaskRunOpts_Execute(t *testing.T) { m.store.EXPECT().GetEnvironment(gomock.Any(), gomock.Any()).AnyTimes() m.deployer.EXPECT().DeployTask(gomock.Any()).AnyTimes() m.repository.EXPECT().Login().Return(mockRepoURI, nil) - m.repository.EXPECT().BuildAndPush(gomock.Eq( + m.repository.EXPECT().BuildAndPush(ctx, gomock.Eq( &dockerengine.BuildArguments{ URI: mockRepoURI, Context: "../../other", Tags: []string{imageTagLatest}, - }), + }), gomock.Any(), ) m.runner.EXPECT().Run().AnyTimes() mockHasDefaultCluster(m) @@ -954,7 +956,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { EntryPoint: []string{"exec", "some command"}, }).Times(1).Return(nil) m.repository.EXPECT().Login().Return(mockRepoURI, nil) - m.repository.EXPECT().BuildAndPush(gomock.Eq(&defaultBuildArguments)) + m.repository.EXPECT().BuildAndPush(ctx, gomock.Eq(&defaultBuildArguments), gomock.Any()) m.deployer.EXPECT().DeployTask(&deploy.CreateTaskResourcesInput{ Name: inGroupName, Image: "uri/repo:latest", diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index f56d94e57fa..1e33d02c714 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -6,8 +6,10 @@ package dockerengine import ( "bytes" + "context" "encoding/json" "fmt" + "io" "os" osexec "os/exec" "path/filepath" @@ -20,6 +22,7 @@ import ( // Cmd is the interface implemented by external commands. type Cmd interface { Run(name string, args []string, options ...exec.CmdOption) error + RunWithContext(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error } // Operating systems and architectures supported by docker. @@ -142,12 +145,12 @@ type dockerConfig struct { } // Build will run a `docker build` command for the given ecr repo URI and build arguments. -func (c DockerCmdClient) Build(in *BuildArguments) error { +func (c DockerCmdClient) Build(ctx context.Context, in *BuildArguments, w io.Writer) error { args, err := in.GenerateDockerBuildArgs(c) if err != nil { return fmt.Errorf("generate docker build args: %w", err) } - if err := c.runner.Run("docker", args); err != nil { + if err := c.runner.RunWithContext(ctx, "docker", args, exec.Stdout(w), exec.Stderr(w)); err != nil { return fmt.Errorf("building image: %w", err) } return nil @@ -167,7 +170,7 @@ func (c DockerCmdClient) Login(uri, username, password string) error { } // Push pushes the images with the specified tags and ecr repository URI, and returns the image digest on success. -func (c DockerCmdClient) Push(uri string, tags ...string) (digest string, err error) { +func (c DockerCmdClient) Push(ctx context.Context, uri string, w io.Writer, tags ...string) (digest string, err error) { images := []string{} for _, tag := range tags { images = append(images, imageName(uri, tag)) @@ -178,7 +181,7 @@ func (c DockerCmdClient) Push(uri string, tags ...string) (digest string, err er } for _, img := range images { - if err := c.runner.Run("docker", append([]string{"push", img}, args...)); err != nil { + if err := c.runner.RunWithContext(ctx, "docker", append([]string{"push", img}, args...), exec.Stdout(w), exec.Stderr(w)); err != nil { return "", fmt.Errorf("docker push %s: %w", img, err) } } @@ -187,7 +190,7 @@ func (c DockerCmdClient) Push(uri string, tags ...string) (digest string, err er // Pick the first tag and get the image's digest. // For Main container we call docker inspect --format '{{json (index .RepoDigests 0)}}' uri:latest // For Sidecar container images we call docker inspect --format '{{json (index .RepoDigests 0)}}' uri:-latest - if err := c.runner.Run("docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", imageName(uri, tags[0])}, exec.Stdout(buf)); err != nil { + if err := c.runner.RunWithContext(ctx, "docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", imageName(uri, tags[0])}, exec.Stdout(buf)); err != nil { return "", fmt.Errorf("inspect image digest for %s: %w", uri, err) } repoDigest := strings.Trim(strings.TrimSpace(buf.String()), `"'`) // remove new lines and quotes from output @@ -289,16 +292,6 @@ func PlatformString(os, arch string) string { return fmt.Sprintf("%s/%s", os, arch) } -// DockerBuildLabel returns the docker build label as a string if platform is not empty. -func DockerBuildLabel(platform string, args []string) string { - // If host platform is not linux/amd64, show the user how the container image is being built; if the build fails (if their docker server doesn't have multi-platform-- and therefore `--platform` capability, for instance) they may see why. - var buildLabel string - if platform != "" { - buildLabel = fmt.Sprintf("Building your container image: docker %s\n", strings.Join(args, " ")) - } - return buildLabel -} - func parseCredFromDockerConfig(config []byte) (*dockerConfig, error) { /* Sample docker config file diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index 593c3ebf873..032d93e8e3d 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -5,6 +5,7 @@ package dockerengine import ( "bytes" + "context" "errors" "fmt" osexec "os/exec" @@ -30,6 +31,7 @@ func TestDockerCommand_Build(t *testing.T) { mockTag2 := "tag2" mockTag3 := "tag3" mockContainerName := "mockWkld" + ctx := context.Background() var mockCmd *MockCmd @@ -60,10 +62,10 @@ func TestDockerCommand_Build(t *testing.T) { tags: []string{mockTag1}, setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", mockURI + ":" + mockTag1, filepath.FromSlash("mockPath/to"), - "-f", "mockPath/to/mockDockerfile"}).Return(mockError) + "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()).Return(mockError) }, wantedError: fmt.Errorf("building image: %w", mockError), }, @@ -74,9 +76,9 @@ func TestDockerCommand_Build(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", "mockURI:tag1", filepath.FromSlash("mockPath/to"), - "-f", "mockPath/to/mockDockerfile"}).Return(nil) + "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()).Return(nil) }, }, "should display quiet progress updates when in a CI environment": { @@ -89,10 +91,10 @@ func TestDockerCommand_Build(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", fmt.Sprintf("%s:%s", mockURI, mockTag1), "--progress", "plain", - filepath.FromSlash("mockPath/to"), "-f", "mockPath/to/mockDockerfile"}). + filepath.FromSlash("mockPath/to"), "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()). Return(nil) }, }, @@ -102,10 +104,10 @@ func TestDockerCommand_Build(t *testing.T) { context: mockContext, setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", fmt.Sprintf("%s:%s", mockURI, mockTag1), "mockPath", - "-f", "mockPath/to/mockDockerfile"}).Return(nil) + "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()).Return(nil) }, }, "behaves the same if context is DF dir": { @@ -115,10 +117,10 @@ func TestDockerCommand_Build(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", mockURI + ":" + mockTag1, "mockPath/to", - "-f", "mockPath/to/mockDockerfile"}).Return(nil) + "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()).Return(nil) }, }, @@ -127,12 +129,12 @@ func TestDockerCommand_Build(t *testing.T) { tags: []string{mockTag1, mockTag2, mockTag3}, setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", mockURI + ":" + mockTag1, "-t", mockURI + ":" + mockTag2, "-t", mockURI + ":" + mockTag3, filepath.FromSlash("mockPath/to"), - "-f", "mockPath/to/mockDockerfile"}).Return(nil) + "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()).Return(nil) }, }, "success with build args": { @@ -145,13 +147,13 @@ func TestDockerCommand_Build(t *testing.T) { }, setupMocks: func(c *gomock.Controller) { mockCmd = NewMockCmd(c) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", fmt.Sprintf("%s:%s", mockURI, "latest"), "--build-arg", "GOPROXY=direct", "--build-arg", "abc=def", "--build-arg", "key=value", filepath.FromSlash("mockPath/to"), - "-f", "mockPath/to/mockDockerfile"}).Return(nil) + "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()).Return(nil) }, }, @@ -165,13 +167,13 @@ func TestDockerCommand_Build(t *testing.T) { }, setupMocks: func(c *gomock.Controller) { mockCmd = NewMockCmd(c) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", fmt.Sprintf("%s:%s", mockURI, "latest"), "--label", "com.aws.copilot.image.builder=copilot-cli", "--label", "com.aws.copilot.image.container.name=mockWkld", "--label", "com.aws.copilot.image.version=v1.26.0", filepath.FromSlash("mockPath/to"), - "-f", "mockPath/to/mockDockerfile"}).Return(nil) + "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()).Return(nil) }, }, @@ -182,13 +184,13 @@ func TestDockerCommand_Build(t *testing.T) { cacheFrom: []string{"foo/bar:latest", "foo/bar/baz:1.2.3"}, setupMocks: func(c *gomock.Controller) { mockCmd = NewMockCmd(c) - mockCmd.EXPECT().Run("docker", []string{"build", + mockCmd.EXPECT().RunWithContext(ctx, "docker", []string{"build", "-t", fmt.Sprintf("%s:%s", mockURI, "latest"), "--cache-from", "foo/bar:latest", "--cache-from", "foo/bar/baz:1.2.3", "--target", "foobar", filepath.FromSlash("mockPath/to"), - "-f", "mockPath/to/mockDockerfile"}).Return(nil) + "-f", "mockPath/to/mockDockerfile"}, gomock.Any(), gomock.Any()).Return(nil) }, }, } @@ -216,7 +218,8 @@ func TestDockerCommand_Build(t *testing.T) { Tags: tc.tags, Labels: tc.labels, } - got := s.Build(&buildInput) + buf := new(strings.Builder) + got := s.Build(ctx, &buildInput, buf) if tc.wantedError != nil { require.EqualError(t, tc.wantedError, got.Error()) @@ -278,15 +281,16 @@ func TestDockerCommand_Push(t *testing.T) { emptyLookupEnv := func(key string) (string, bool) { return "", false } + ctx := context.Background() t.Run("pushes an image with multiple tags and returns its digest", func(t *testing.T) { // GIVEN ctrl := gomock.NewController(t) defer ctrl.Finish() m := NewMockCmd(ctrl) - m.EXPECT().Run("docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}).Return(nil) - m.EXPECT().Run("docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:g123bfc"}).Return(nil) - m.EXPECT().Run("docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}, gomock.Any()). - Do(func(_ string, _ []string, opt exec.CmdOption) { + m.EXPECT().RunWithContext(ctx, "docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}, gomock.Any(), gomock.Any()).Return(nil) + m.EXPECT().RunWithContext(ctx, "docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:g123bfc"}, gomock.Any(), gomock.Any()).Return(nil) + m.EXPECT().RunWithContext(ctx, "docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}, gomock.Any()). + Do(func(ctx context.Context, _ string, _ []string, opt exec.CmdOption) { cmd := &osexec.Cmd{} opt(cmd) _, _ = cmd.Stdout.Write([]byte("\"aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app@sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807\"\n")) @@ -297,7 +301,8 @@ func TestDockerCommand_Push(t *testing.T) { runner: m, lookupEnv: emptyLookupEnv, } - digest, err := cmd.Push("aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app", "latest", "g123bfc") + buf := new(strings.Builder) + digest, err := cmd.Push(ctx, "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app", buf, "latest", "g123bfc") // THEN require.NoError(t, err) @@ -308,9 +313,9 @@ func TestDockerCommand_Push(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() m := NewMockCmd(ctrl) - m.EXPECT().Run("docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest", "--quiet"}).Return(nil) - m.EXPECT().Run("docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}, gomock.Any()). - Do(func(_ string, _ []string, opt exec.CmdOption) { + m.EXPECT().RunWithContext(ctx, "docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest", "--quiet"}, gomock.Any(), gomock.Any()).Return(nil) + m.EXPECT().RunWithContext(ctx, "docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}, gomock.Any()). + Do(func(ctx context.Context, _ string, _ []string, opt exec.CmdOption) { cmd := &osexec.Cmd{} opt(cmd) _, _ = cmd.Stdout.Write([]byte("\"aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app@sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807\"\n")) @@ -326,7 +331,8 @@ func TestDockerCommand_Push(t *testing.T) { return "", false }, } - digest, err := cmd.Push("aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app", "latest") + buf := new(strings.Builder) + digest, err := cmd.Push(context.Background(), "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app", buf, "latest") // THEN require.NoError(t, err) @@ -337,14 +343,15 @@ func TestDockerCommand_Push(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() m := NewMockCmd(ctrl) - m.EXPECT().Run(gomock.Any(), gomock.Any()).Return(errors.New("some error")) + m.EXPECT().RunWithContext(ctx, gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("some error")) // WHEN cmd := DockerCmdClient{ runner: m, lookupEnv: emptyLookupEnv, } - _, err := cmd.Push("uri", "latest") + buf := new(strings.Builder) + _, err := cmd.Push(ctx, "uri", buf, "latest") // THEN require.EqualError(t, err, "docker push uri:latest: some error") @@ -354,15 +361,16 @@ func TestDockerCommand_Push(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() m := NewMockCmd(ctrl) - m.EXPECT().Run("docker", []string{"push", "uri:latest"}).Return(nil) - m.EXPECT().Run("docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", "uri:latest"}, gomock.Any()).Return(errors.New("some error")) + m.EXPECT().RunWithContext(ctx, "docker", []string{"push", "uri:latest"}, gomock.Any(), gomock.Any()).Return(nil) + m.EXPECT().RunWithContext(ctx, "docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", "uri:latest"}, gomock.Any()).Return(errors.New("some error")) // WHEN cmd := DockerCmdClient{ runner: m, lookupEnv: emptyLookupEnv, } - _, err := cmd.Push("uri", "latest") + buf := new(strings.Builder) + _, err := cmd.Push(ctx, "uri", buf, "latest") // THEN require.EqualError(t, err, "inspect image digest for uri: some error") @@ -372,10 +380,10 @@ func TestDockerCommand_Push(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() m := NewMockCmd(ctrl) - m.EXPECT().Run("docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}).Return(nil) - m.EXPECT().Run("docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:g123bfc"}).Return(nil) - m.EXPECT().Run("docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}, gomock.Any()). - Do(func(_ string, _ []string, opt exec.CmdOption) { + m.EXPECT().RunWithContext(ctx, "docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}, gomock.Any(), gomock.Any()).Return(nil) + m.EXPECT().RunWithContext(ctx, "docker", []string{"push", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:g123bfc"}, gomock.Any(), gomock.Any()).Return(nil) + m.EXPECT().RunWithContext(ctx, "docker", []string{"inspect", "--format", "'{{json (index .RepoDigests 0)}}'", "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app:latest"}, gomock.Any()). + Do(func(ctx context.Context, _ string, _ []string, opt exec.CmdOption) { cmd := &osexec.Cmd{} opt(cmd) _, _ = cmd.Stdout.Write([]byte("")) @@ -386,7 +394,8 @@ func TestDockerCommand_Push(t *testing.T) { runner: m, lookupEnv: emptyLookupEnv, } - _, err := cmd.Push("aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app", "latest", "g123bfc") + buf := new(strings.Builder) + _, err := cmd.Push(ctx, "aws_account_id.dkr.ecr.region.amazonaws.com/my-web-app", buf, "latest", "g123bfc") // THEN require.EqualError(t, err, "parse the digest from the repo digest ''") @@ -614,33 +623,3 @@ func TestIsEcrCredentialHelperEnabled(t *testing.T) { }) } } - -func Test_DockerBuildLabel(t *testing.T) { - testCases := map[string]struct { - inBuildArgs []string - inPlatform string - wanted string - }{ - "if platform is not empty": { - inBuildArgs: []string{"build", "-t", "mockURI:tag1", - filepath.FromSlash("mockPath/to"), "-f", "mockPath/to/mockDockerfile", - }, - inPlatform: "windows/x86_64", - wanted: fmt.Sprintf("Building your container image: docker %s\n", - strings.Join([]string{"build", "-t", "mockURI:tag1", - filepath.FromSlash("mockPath/to"), "-f", "mockPath/to/mockDockerfile"}, " ")), - }, - "if platform is empty": { - inBuildArgs: []string{"build", "-t", "mockURI:tag1", - filepath.FromSlash("mockPath/to"), "-f", "mockPath/to/mockDockerfile", - }, - }, - } - for name, tc := range testCases { - t.Run(name, func(t *testing.T) { - got := DockerBuildLabel(tc.inPlatform, tc.inBuildArgs) - require.Equal(t, tc.wanted, got) - - }) - } -} diff --git a/internal/pkg/docker/dockerengine/mock_dockerengine.go b/internal/pkg/docker/dockerengine/mock_dockerengine.go index 6d3e96bb909..a0b0d3b10b6 100644 --- a/internal/pkg/docker/dockerengine/mock_dockerengine.go +++ b/internal/pkg/docker/dockerengine/mock_dockerengine.go @@ -5,6 +5,7 @@ package dockerengine import ( + context "context" reflect "reflect" exec "github.com/aws/copilot-cli/internal/pkg/exec" @@ -52,3 +53,22 @@ func (mr *MockCmdMockRecorder) Run(name, args interface{}, options ...interface{ varargs := append([]interface{}{name, args}, options...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Run", reflect.TypeOf((*MockCmd)(nil).Run), varargs...) } + +// RunWithContext mocks base method. +func (m *MockCmd) RunWithContext(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + m.ctrl.T.Helper() + varargs := []interface{}{ctx, name, args} + for _, a := range opts { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "RunWithContext", varargs...) + ret0, _ := ret[0].(error) + return ret0 +} + +// RunWithContext indicates an expected call of RunWithContext. +func (mr *MockCmdMockRecorder) RunWithContext(ctx, name, args interface{}, opts ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{ctx, name, args}, opts...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RunWithContext", reflect.TypeOf((*MockCmd)(nil).RunWithContext), varargs...) +} diff --git a/internal/pkg/repository/mocks/mock_repository.go b/internal/pkg/repository/mocks/mock_repository.go index c4f9bc1e063..b5ef4a6afcf 100644 --- a/internal/pkg/repository/mocks/mock_repository.go +++ b/internal/pkg/repository/mocks/mock_repository.go @@ -5,6 +5,8 @@ package mocks import ( + context "context" + io "io" reflect "reflect" dockerengine "github.com/aws/copilot-cli/internal/pkg/docker/dockerengine" @@ -35,17 +37,17 @@ func (m *MockContainerLoginBuildPusher) EXPECT() *MockContainerLoginBuildPusherM } // Build mocks base method. -func (m *MockContainerLoginBuildPusher) Build(args *dockerengine.BuildArguments) error { +func (m *MockContainerLoginBuildPusher) Build(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Build", args) + ret := m.ctrl.Call(m, "Build", ctx, args, w) ret0, _ := ret[0].(error) return ret0 } // Build indicates an expected call of Build. -func (mr *MockContainerLoginBuildPusherMockRecorder) Build(args interface{}) *gomock.Call { +func (mr *MockContainerLoginBuildPusherMockRecorder) Build(ctx, args, w interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Build", reflect.TypeOf((*MockContainerLoginBuildPusher)(nil).Build), args) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Build", reflect.TypeOf((*MockContainerLoginBuildPusher)(nil).Build), ctx, args, w) } // IsEcrCredentialHelperEnabled mocks base method. @@ -77,9 +79,9 @@ func (mr *MockContainerLoginBuildPusherMockRecorder) Login(uri, username, passwo } // Push mocks base method. -func (m *MockContainerLoginBuildPusher) Push(uri string, tags ...string) (string, error) { +func (m *MockContainerLoginBuildPusher) Push(ctx context.Context, uri string, w io.Writer, tags ...string) (string, error) { m.ctrl.T.Helper() - varargs := []interface{}{uri} + varargs := []interface{}{ctx, uri, w} for _, a := range tags { varargs = append(varargs, a) } @@ -90,9 +92,9 @@ func (m *MockContainerLoginBuildPusher) Push(uri string, tags ...string) (string } // Push indicates an expected call of Push. -func (mr *MockContainerLoginBuildPusherMockRecorder) Push(uri interface{}, tags ...interface{}) *gomock.Call { +func (mr *MockContainerLoginBuildPusherMockRecorder) Push(ctx, uri, w interface{}, tags ...interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - varargs := append([]interface{}{uri}, tags...) + varargs := append([]interface{}{ctx, uri, w}, tags...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Push", reflect.TypeOf((*MockContainerLoginBuildPusher)(nil).Push), varargs...) } diff --git a/internal/pkg/repository/repository.go b/internal/pkg/repository/repository.go index 3247a4bf5d9..08b354703f7 100644 --- a/internal/pkg/repository/repository.go +++ b/internal/pkg/repository/repository.go @@ -5,7 +5,9 @@ package repository import ( + "context" "fmt" + "io" "github.com/aws/copilot-cli/internal/pkg/exec" @@ -14,9 +16,9 @@ import ( // ContainerLoginBuildPusher provides support for logging in to repositories, building images and pushing images to repositories. type ContainerLoginBuildPusher interface { - Build(args *dockerengine.BuildArguments) error + Build(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) error Login(uri, username, password string) error - Push(uri string, tags ...string) (digest string, err error) + Push(ctx context.Context, uri string, w io.Writer, tags ...string) (digest string, err error) IsEcrCredentialHelperEnabled(uri string) bool } @@ -54,7 +56,7 @@ func NewWithURI(registry Registry, name, uri string) *Repository { } // BuildAndPush builds the image from Dockerfile and pushes it to the repository with tags. -func (r *Repository) BuildAndPush(args *dockerengine.BuildArguments) (digest string, err error) { +func (r *Repository) BuildAndPush(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) (digest string, err error) { if args.URI == "" { uri, err := r.repositoryURI() if err != nil { @@ -62,11 +64,11 @@ func (r *Repository) BuildAndPush(args *dockerengine.BuildArguments) (digest str } args.URI = uri } - if err := r.docker.Build(args); err != nil { + if err := r.docker.Build(ctx, args, w); err != nil { return "", fmt.Errorf("build Dockerfile at %s: %w", args.Dockerfile, err) } - digest, err = r.docker.Push(args.URI, args.Tags...) + digest, err = r.docker.Push(ctx, args.URI, w, args.Tags...) if err != nil { return "", fmt.Errorf("push to repo %s: %w", r.name, err) } diff --git a/internal/pkg/repository/repository_test.go b/internal/pkg/repository/repository_test.go index 8a2fd0c8f39..0d86abe2545 100644 --- a/internal/pkg/repository/repository_test.go +++ b/internal/pkg/repository/repository_test.go @@ -4,9 +4,11 @@ package repository import ( + "context" "errors" "fmt" "path/filepath" + "strings" "testing" "github.com/aws/copilot-cli/internal/pkg/docker/dockerengine" @@ -22,6 +24,7 @@ func TestRepository_BuildAndPush(t *testing.T) { mockTag1, mockTag2, mockTag3 := "tag1", "tag2", "tag3" mockRepoURI := "mockRepoURI" + ctx := context.Background() defaultDockerArguments := dockerengine.BuildArguments{ URI: mockRepoURI, @@ -52,24 +55,23 @@ func TestRepository_BuildAndPush(t *testing.T) { m.EXPECT().Auth().Return("", "", nil).AnyTimes() }, inMockDocker: func(m *mocks.MockContainerLoginBuildPusher) { - m.EXPECT().Build(&defaultDockerArguments).Return(errors.New("error building image")) - m.EXPECT().Push(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + m.EXPECT().Build(ctx, &defaultDockerArguments, gomock.Any()).Return(errors.New("error building image")) }, wantedError: fmt.Errorf("build Dockerfile at %s: error building image", inDockerfilePath), }, "failed to push": { inURI: defaultDockerArguments.URI, inMockDocker: func(m *mocks.MockContainerLoginBuildPusher) { - m.EXPECT().Build(&defaultDockerArguments).Times(1) - m.EXPECT().Push(mockRepoURI, mockTag1, mockTag2, mockTag3).Return("", errors.New("error pushing image")) + m.EXPECT().Build(ctx, &defaultDockerArguments, gomock.Any()).Times(1) + m.EXPECT().Push(ctx, mockRepoURI, gomock.Any(), mockTag1, mockTag2, mockTag3).Return("", errors.New("error pushing image")) }, wantedError: errors.New("push to repo my-repo: error pushing image"), }, "push with ecr-login": { inURI: defaultDockerArguments.URI, inMockDocker: func(m *mocks.MockContainerLoginBuildPusher) { - m.EXPECT().Build(&defaultDockerArguments).Return(nil).Times(1) - m.EXPECT().Push(mockRepoURI, mockTag1, mockTag2, mockTag3).Return("sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807", nil) + m.EXPECT().Build(ctx, &defaultDockerArguments, gomock.Any()).Return(nil).Times(1) + m.EXPECT().Push(ctx, mockRepoURI, gomock.Any(), mockTag1, mockTag2, mockTag3).Return("sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807", nil) }, wantedDigest: "sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807", }, @@ -78,8 +80,8 @@ func TestRepository_BuildAndPush(t *testing.T) { m.EXPECT().RepositoryURI(inRepoName).Return(defaultDockerArguments.URI, nil) }, inMockDocker: func(m *mocks.MockContainerLoginBuildPusher) { - m.EXPECT().Build(&defaultDockerArguments).Return(nil).Times(1) - m.EXPECT().Push(mockRepoURI, mockTag1, mockTag2, mockTag3).Return("sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807", nil) + m.EXPECT().Build(ctx, &defaultDockerArguments, gomock.Any()).Return(nil).Times(1) + m.EXPECT().Push(ctx, mockRepoURI, gomock.Any(), mockTag1, mockTag2, mockTag3).Return("sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807", nil) }, wantedDigest: "sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807", }, @@ -105,12 +107,12 @@ func TestRepository_BuildAndPush(t *testing.T) { uri: tc.inURI, docker: mockDocker, } - - digest, err := repo.BuildAndPush(&dockerengine.BuildArguments{ + buf := new(strings.Builder) + digest, err := repo.BuildAndPush(ctx, &dockerengine.BuildArguments{ Dockerfile: inDockerfilePath, Context: filepath.Dir(inDockerfilePath), Tags: []string{mockTag1, mockTag2, mockTag3}, - }) + }, buf) if tc.wantedError != nil { require.EqualError(t, tc.wantedError, err.Error()) } else { diff --git a/internal/pkg/term/syncbuffer/syncbuffer.go b/internal/pkg/term/syncbuffer/syncbuffer.go index f9d7ca5ae78..e940b695fee 100644 --- a/internal/pkg/term/syncbuffer/syncbuffer.go +++ b/internal/pkg/term/syncbuffer/syncbuffer.go @@ -65,7 +65,7 @@ func (buf *SyncBuffer) WithLabel(label string) *LabeledSyncBuffer { // Copy reads all the content of an io.Reader into a SyncBuffer and an error if copy is failed. func (buf *SyncBuffer) Copy(r io.Reader) error { defer buf.MarkDone() - _, err := io.Copy(&buf.buf, r) + _, err := io.Copy(buf, r) if err != nil && !errors.Is(err, io.EOF) { return err } From 82333d4bf776ef10fc1068b5866ad0aea3be5b0e Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Thu, 13 Apr 2023 13:59:37 -0700 Subject: [PATCH 06/24] remove \n from label --- internal/pkg/cli/deploy/workload.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 515d8d82990..f0b0e10cf59 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -419,7 +419,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err return fmt.Errorf("generate docker build args: %w", err) } buf := syncbuffer.New() - labeledBuffer := buf.WithLabel(fmt.Sprintf("Building your container image: docker %s\n", strings.Join(buildArgsList, " "))) + labeledBuffer := buf.WithLabel(fmt.Sprintf("Building your container image: docker %s", strings.Join(buildArgsList, " "))) labeledBuffers[count] = labeledBuffer count++ From 6f58be0eadf71b732e57298f5837b3ef561e24c4 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Thu, 13 Apr 2023 14:35:30 -0700 Subject: [PATCH 07/24] remove unused mock --- internal/pkg/cli/deploy/workload_test.go | 12 +-- .../term/syncbuffer/mocks/mock_termprinter.go | 102 ++++++++++++++++++ internal/pkg/term/syncbuffer/termprinter.go | 4 + 3 files changed, 109 insertions(+), 9 deletions(-) create mode 100644 internal/pkg/term/syncbuffer/mocks/mock_termprinter.go diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index d84c1746d47..736b946a263 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -32,7 +32,6 @@ import ( "github.com/aws/copilot-cli/internal/pkg/template" "github.com/aws/copilot-cli/internal/pkg/term/color" "github.com/aws/copilot-cli/internal/pkg/term/log" - "github.com/aws/copilot-cli/internal/pkg/term/syncbuffer" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" ) @@ -185,10 +184,9 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { inMockGitTag string inDockerBuildArgs map[string]*manifest.DockerBuildArgs - mock func(t *testing.T, m *deployMocks) - mockServiceDeployer func(deployer *workloadDeployer) artifactsUploader - customResourcesFunc customResourcesFunc - mockTerminalWidthFunc func(fw syncbuffer.FileWriter) (int, error) + mock func(t *testing.T, m *deployMocks) + mockServiceDeployer func(deployer *workloadDeployer) artifactsUploader + customResourcesFunc customResourcesFunc wantAddonsURL string wantEnvFileARNs map[string]string @@ -609,10 +607,6 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { return nil, nil } } - // mockTerminalWidth := func(fw syncbuffer.FileWriter) (int, error) { - // return 80, nil - // } - wkldDeployer := &workloadDeployer{ name: mockName, env: &config.Environment{ diff --git a/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go b/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go new file mode 100644 index 00000000000..42d9666eac3 --- /dev/null +++ b/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go @@ -0,0 +1,102 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: ./internal/pkg/term/syncbuffer/termprinter.go + +// Package mocks is a generated GoMock package. +package mocks + +import ( + reflect "reflect" + + syncbuffer "github.com/aws/copilot-cli/internal/pkg/term/syncbuffer" + gomock "github.com/golang/mock/gomock" +) + +// MockFileWriter is a mock of FileWriter interface. +type MockFileWriter struct { + ctrl *gomock.Controller + recorder *MockFileWriterMockRecorder +} + +// MockFileWriterMockRecorder is the mock recorder for MockFileWriter. +type MockFileWriterMockRecorder struct { + mock *MockFileWriter +} + +// NewMockFileWriter creates a new mock instance. +func NewMockFileWriter(ctrl *gomock.Controller) *MockFileWriter { + mock := &MockFileWriter{ctrl: ctrl} + mock.recorder = &MockFileWriterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockFileWriter) EXPECT() *MockFileWriterMockRecorder { + return m.recorder +} + +// Fd mocks base method. +func (m *MockFileWriter) Fd() uintptr { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Fd") + ret0, _ := ret[0].(uintptr) + return ret0 +} + +// Fd indicates an expected call of Fd. +func (mr *MockFileWriterMockRecorder) Fd() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Fd", reflect.TypeOf((*MockFileWriter)(nil).Fd)) +} + +// Write mocks base method. +func (m *MockFileWriter) Write(p []byte) (int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Write", p) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Write indicates an expected call of Write. +func (mr *MockFileWriterMockRecorder) Write(p interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Write", reflect.TypeOf((*MockFileWriter)(nil).Write), p) +} + +// MockTerminal is a mock of Terminal interface. +type MockTerminal struct { + ctrl *gomock.Controller + recorder *MockTerminalMockRecorder +} + +// MockTerminalMockRecorder is the mock recorder for MockTerminal. +type MockTerminalMockRecorder struct { + mock *MockTerminal +} + +// NewMockTerminal creates a new mock instance. +func NewMockTerminal(ctrl *gomock.Controller) *MockTerminal { + mock := &MockTerminal{ctrl: ctrl} + mock.recorder = &MockTerminalMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockTerminal) EXPECT() *MockTerminalMockRecorder { + return m.recorder +} + +// terminalWidth mocks base method. +func (m *MockTerminal) terminalWidth(w syncbuffer.FileWriter) (int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "terminalWidth", w) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// terminalWidth indicates an expected call of terminalWidth. +func (mr *MockTerminalMockRecorder) terminalWidth(w interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "terminalWidth", reflect.TypeOf((*MockTerminal)(nil).terminalWidth), w) +} diff --git a/internal/pkg/term/syncbuffer/termprinter.go b/internal/pkg/term/syncbuffer/termprinter.go index aa8581ed3d0..58457363b41 100644 --- a/internal/pkg/term/syncbuffer/termprinter.go +++ b/internal/pkg/term/syncbuffer/termprinter.go @@ -25,6 +25,10 @@ type FileWriter interface { Fd() uintptr } +type Terminal interface { + terminalWidth(w FileWriter) (int, error) +} + // LabeledTermPrinter is a printer to display label and logs to the terminal. type LabeledTermPrinter struct { term FileWriter // term writes logs to the terminal FileWriter. From 11ef4d8d9864cbdf3c5f0deccdd6b3913b1577b7 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Thu, 13 Apr 2023 14:39:56 -0700 Subject: [PATCH 08/24] remove some unused termprinter --- internal/pkg/cli/deploy/workload_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index 736b946a263..89cd9221842 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -202,7 +202,6 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { Context: aws.String("mockContext"), }, }, - // mockTerminalWidthFunc: , mock: func(t *testing.T, m *deployMocks) { m.mockRepositoryService.EXPECT().Login().Return(mockURI, nil) m.mockRepositoryService.EXPECT().BuildAndPush(gomock.Any(), &dockerengine.BuildArguments{ From e6e6f616090f649a4eb6633bfae288bd09536c7e Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Thu, 13 Apr 2023 15:44:34 -0700 Subject: [PATCH 09/24] addr Parag fb: addd name of image in error stmt --- internal/pkg/cli/deploy/workload.go | 14 +++++++------- internal/pkg/cli/task_run.go | 2 +- internal/pkg/term/syncbuffer/termprinter.go | 4 ---- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index f0b0e10cf59..13a1eabed49 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -392,8 +392,8 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err out.ImageDigests = make(map[string]ContainerImageIdentifier, len(buildArgsPerContainer)) // counter for indexing the labeled output buffers. - count := 0 - // An array of buffers, one for each container image build and push ouput. + bufferIdx := 0 + // An array of buffers, one for each container image build and push output. labeledBuffers := make([]*syncbuffer.LabeledSyncBuffer, len(buildArgsPerContainer)) // create a context and an error group. @@ -416,18 +416,18 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err // generate build args slice for docker build label. buildArgsList, err := buildArgs.GenerateDockerBuildArgs(dockerengine.New(exec.NewCmd())) if err != nil { - return fmt.Errorf("generate docker build args: %w", err) + return fmt.Errorf("generate docker build args of image %s: %w", name, err) } buf := syncbuffer.New() - labeledBuffer := buf.WithLabel(fmt.Sprintf("Building your container image: docker %s", strings.Join(buildArgsList, " "))) - labeledBuffers[count] = labeledBuffer - count++ + labeledBuffer := buf.WithLabel(fmt.Sprintf("Building your container image %s: docker %s", name, strings.Join(buildArgsList, " "))) + labeledBuffers[bufferIdx] = labeledBuffer + bufferIdx++ g.Go(func() error { defer pw.Close() digest, err := d.repository.BuildAndPush(ctx, buildArgs, pw) if err != nil { - return fmt.Errorf("build and push image: %w", err) + return fmt.Errorf("build and push image %s: %w", name, err) } mu.Lock() out.ImageDigests[name] = ContainerImageIdentifier{ diff --git a/internal/pkg/cli/task_run.go b/internal/pkg/cli/task_run.go index 87e7042333c..a4a8100ac35 100644 --- a/internal/pkg/cli/task_run.go +++ b/internal/pkg/cli/task_run.go @@ -973,7 +973,7 @@ func (o *runTaskOpts) buildAndPushImage(uri string) error { if err != nil { return fmt.Errorf("generate docker build args: %w", err) } - log.Infof(fmt.Sprintf("Building your container image: docker %s\n", strings.Join(buildArgsList, " "))) + log.Infof("Building your container image: docker %s\n", strings.Join(buildArgsList, " ")) if _, err := o.repository.BuildAndPush(context.Background(), buildArgs, log.DiagnosticWriter); err != nil { return fmt.Errorf("build and push image: %w", err) } diff --git a/internal/pkg/term/syncbuffer/termprinter.go b/internal/pkg/term/syncbuffer/termprinter.go index 58457363b41..aa8581ed3d0 100644 --- a/internal/pkg/term/syncbuffer/termprinter.go +++ b/internal/pkg/term/syncbuffer/termprinter.go @@ -25,10 +25,6 @@ type FileWriter interface { Fd() uintptr } -type Terminal interface { - terminalWidth(w FileWriter) (int, error) -} - // LabeledTermPrinter is a printer to display label and logs to the terminal. type LabeledTermPrinter struct { term FileWriter // term writes logs to the terminal FileWriter. From 13665e2d03c5957fbf78ec865c48b25671b9e9e9 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Thu, 13 Apr 2023 16:02:26 -0700 Subject: [PATCH 10/24] fix test --- internal/pkg/cli/deploy/workload_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index 89cd9221842..930c04fbcfa 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -216,7 +216,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { }, }, gomock.Any()).Return("", mockError) }, - wantErr: fmt.Errorf("build and push image: some error"), + wantErr: fmt.Errorf("build and push image mockWkld: some error"), }, "build and push image with usertag successfully": { inMockUserTag: "v1.0", From 4478437d230e9572bfa5e36a4ebb84b6e0193762 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Fri, 14 Apr 2023 10:38:08 -0700 Subject: [PATCH 11/24] remove mocks --- .../term/syncbuffer/mocks/mock_termprinter.go | 39 ------------------- 1 file changed, 39 deletions(-) diff --git a/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go b/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go index 42d9666eac3..04f26e19278 100644 --- a/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go +++ b/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go @@ -7,7 +7,6 @@ package mocks import ( reflect "reflect" - syncbuffer "github.com/aws/copilot-cli/internal/pkg/term/syncbuffer" gomock "github.com/golang/mock/gomock" ) @@ -62,41 +61,3 @@ func (mr *MockFileWriterMockRecorder) Write(p interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Write", reflect.TypeOf((*MockFileWriter)(nil).Write), p) } - -// MockTerminal is a mock of Terminal interface. -type MockTerminal struct { - ctrl *gomock.Controller - recorder *MockTerminalMockRecorder -} - -// MockTerminalMockRecorder is the mock recorder for MockTerminal. -type MockTerminalMockRecorder struct { - mock *MockTerminal -} - -// NewMockTerminal creates a new mock instance. -func NewMockTerminal(ctrl *gomock.Controller) *MockTerminal { - mock := &MockTerminal{ctrl: ctrl} - mock.recorder = &MockTerminalMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockTerminal) EXPECT() *MockTerminalMockRecorder { - return m.recorder -} - -// terminalWidth mocks base method. -func (m *MockTerminal) terminalWidth(w syncbuffer.FileWriter) (int, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "terminalWidth", w) - ret0, _ := ret[0].(int) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// terminalWidth indicates an expected call of terminalWidth. -func (mr *MockTerminalMockRecorder) terminalWidth(w interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "terminalWidth", reflect.TypeOf((*MockTerminal)(nil).terminalWidth), w) -} From c19956788e39561d545f59b9069d0035e2ae7738 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Fri, 14 Apr 2023 10:41:19 -0700 Subject: [PATCH 12/24] remove unused mocks --- .../term/syncbuffer/mocks/mock_termprinter.go | 63 ------------------- 1 file changed, 63 deletions(-) delete mode 100644 internal/pkg/term/syncbuffer/mocks/mock_termprinter.go diff --git a/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go b/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go deleted file mode 100644 index 04f26e19278..00000000000 --- a/internal/pkg/term/syncbuffer/mocks/mock_termprinter.go +++ /dev/null @@ -1,63 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: ./internal/pkg/term/syncbuffer/termprinter.go - -// Package mocks is a generated GoMock package. -package mocks - -import ( - reflect "reflect" - - gomock "github.com/golang/mock/gomock" -) - -// MockFileWriter is a mock of FileWriter interface. -type MockFileWriter struct { - ctrl *gomock.Controller - recorder *MockFileWriterMockRecorder -} - -// MockFileWriterMockRecorder is the mock recorder for MockFileWriter. -type MockFileWriterMockRecorder struct { - mock *MockFileWriter -} - -// NewMockFileWriter creates a new mock instance. -func NewMockFileWriter(ctrl *gomock.Controller) *MockFileWriter { - mock := &MockFileWriter{ctrl: ctrl} - mock.recorder = &MockFileWriterMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockFileWriter) EXPECT() *MockFileWriterMockRecorder { - return m.recorder -} - -// Fd mocks base method. -func (m *MockFileWriter) Fd() uintptr { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Fd") - ret0, _ := ret[0].(uintptr) - return ret0 -} - -// Fd indicates an expected call of Fd. -func (mr *MockFileWriterMockRecorder) Fd() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Fd", reflect.TypeOf((*MockFileWriter)(nil).Fd)) -} - -// Write mocks base method. -func (m *MockFileWriter) Write(p []byte) (int, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Write", p) - ret0, _ := ret[0].(int) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Write indicates an expected call of Write. -func (mr *MockFileWriterMockRecorder) Write(p interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Write", reflect.TypeOf((*MockFileWriter)(nil).Write), p) -} From 526a90625f0a12886d73e96fd0cc6a3297f16330 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Mon, 17 Apr 2023 01:15:14 -0700 Subject: [PATCH 13/24] add mocks for labeled termprinter print --- .../pkg/cli/deploy/mocks/mock_workload.go | 51 +++++++++++++++++++ internal/pkg/cli/deploy/workload.go | 37 +++++++++----- internal/pkg/cli/deploy/workload_test.go | 22 ++++++-- 3 files changed, 93 insertions(+), 17 deletions(-) diff --git a/internal/pkg/cli/deploy/mocks/mock_workload.go b/internal/pkg/cli/deploy/mocks/mock_workload.go index f5c051cb987..2ebdf7dfa91 100644 --- a/internal/pkg/cli/deploy/mocks/mock_workload.go +++ b/internal/pkg/cli/deploy/mocks/mock_workload.go @@ -444,6 +444,57 @@ func (mr *MockspinnerMockRecorder) Stop(label interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Stop", reflect.TypeOf((*Mockspinner)(nil).Stop), label) } +// MocklabeledTermPrinter is a mock of labeledTermPrinter interface. +type MocklabeledTermPrinter struct { + ctrl *gomock.Controller + recorder *MocklabeledTermPrinterMockRecorder +} + +// MocklabeledTermPrinterMockRecorder is the mock recorder for MocklabeledTermPrinter. +type MocklabeledTermPrinterMockRecorder struct { + mock *MocklabeledTermPrinter +} + +// NewMocklabeledTermPrinter creates a new mock instance. +func NewMocklabeledTermPrinter(ctrl *gomock.Controller) *MocklabeledTermPrinter { + mock := &MocklabeledTermPrinter{ctrl: ctrl} + mock.recorder = &MocklabeledTermPrinterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MocklabeledTermPrinter) EXPECT() *MocklabeledTermPrinterMockRecorder { + return m.recorder +} + +// IsDone mocks base method. +func (m *MocklabeledTermPrinter) IsDone() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsDone") + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsDone indicates an expected call of IsDone. +func (mr *MocklabeledTermPrinterMockRecorder) IsDone() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsDone", reflect.TypeOf((*MocklabeledTermPrinter)(nil).IsDone)) +} + +// Print mocks base method. +func (m *MocklabeledTermPrinter) Print() error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Print") + ret0, _ := ret[0].(error) + return ret0 +} + +// Print indicates an expected call of Print. +func (mr *MocklabeledTermPrinterMockRecorder) Print() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Print", reflect.TypeOf((*MocklabeledTermPrinter)(nil).Print)) +} + // MocktimeoutError is a mock of timeoutError interface. type MocktimeoutError struct { ctrl *gomock.Controller diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 13a1eabed49..0ec89b4216b 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -119,6 +119,11 @@ type spinner interface { Stop(label string) } +type labeledTermPrinter interface { + IsDone() bool + Print() error +} + // StackRuntimeConfiguration contains runtime configuration for a workload CloudFormation stack. type StackRuntimeConfiguration struct { ImageDigests map[string]ContainerImageIdentifier // Container name to image. @@ -153,6 +158,8 @@ type GenerateCloudFormationTemplateOutput struct { Parameters string } +type labeledTermPrinterFunc func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter + type workloadDeployer struct { name string app *config.Application @@ -165,18 +172,19 @@ type workloadDeployer struct { uri string // Dependencies. - fs afero.Fs - s3Client uploader - addons stackBuilder - repository repositoryService - deployer serviceDeployer - tmplGetter deployedTemplateGetter - endpointGetter endpointGetter - spinner spinner - templateFS template.Reader - envVersionGetter versionGetter - overrider Overrider - customResources customResourcesFunc + fs afero.Fs + s3Client uploader + addons stackBuilder + repository repositoryService + deployer serviceDeployer + tmplGetter deployedTemplateGetter + endpointGetter endpointGetter + spinner spinner + templateFS template.Reader + envVersionGetter versionGetter + overrider Overrider + customResources customResourcesFunc + labeledTermPrinter labeledTermPrinterFunc // Cached variables. defaultSess *session.Session @@ -295,6 +303,9 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { envSess: envSession, store: store, envConfig: envConfig, + labeledTermPrinter: func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter { + return syncbuffer.NewLabeledTermPrinter(fw, bufs, opts...) + }, mft: in.Mft, rawMft: in.RawMft, @@ -449,7 +460,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err numLinesForBuildAndPush = -1 } // create a LabeledTermPrinter for rendering build and push output. - ltp := syncbuffer.NewLabeledTermPrinter(os.Stderr, labeledBuffers, syncbuffer.WithNumLines(numLinesForBuildAndPush), syncbuffer.WithPadding(paddingForBuildAndPush)) + ltp := d.labeledTermPrinter(os.Stderr, labeledBuffers, syncbuffer.WithNumLines(numLinesForBuildAndPush), syncbuffer.WithPadding(paddingForBuildAndPush)) g.Go(func() error { for { diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index 930c04fbcfa..f4f6239275f 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -32,6 +32,7 @@ import ( "github.com/aws/copilot-cli/internal/pkg/template" "github.com/aws/copilot-cli/internal/pkg/term/color" "github.com/aws/copilot-cli/internal/pkg/term/log" + "github.com/aws/copilot-cli/internal/pkg/term/syncbuffer" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" ) @@ -50,6 +51,7 @@ type deployMocks struct { mockEnvVersionGetter *mocks.MockversionGetter mockFileSystem afero.Fs mockValidator *mocks.MockaliasCertValidator + mockLabeledTermPrinter *mocks.MocklabeledTermPrinter } type mockTemplateFS struct { @@ -215,6 +217,8 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.container.name": "mockWkld", }, }, gomock.Any()).Return("", mockError) + m.mockLabeledTermPrinter.EXPECT().IsDone().Return(true).AnyTimes() + m.mockLabeledTermPrinter.EXPECT().Print().Return(nil).AnyTimes() }, wantErr: fmt.Errorf("build and push image mockWkld: some error"), }, @@ -240,6 +244,8 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.container.name": "mockWkld", }, }, gomock.Any()).Return("mockDigest", nil) + m.mockLabeledTermPrinter.EXPECT().IsDone().Return(true).AnyTimes() + m.mockLabeledTermPrinter.EXPECT().Print().Return(nil).AnyTimes() m.mockAddons = nil }, wantImages: map[string]ContainerImageIdentifier{ @@ -271,6 +277,8 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.container.name": "mockWkld", }, }, gomock.Any()).Return("mockDigest", nil) + m.mockLabeledTermPrinter.EXPECT().IsDone().Return(true).AnyTimes() + m.mockLabeledTermPrinter.EXPECT().Print().Return(nil).AnyTimes() m.mockAddons = nil }, wantImages: map[string]ContainerImageIdentifier{ @@ -316,6 +324,8 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { "com.aws.copilot.image.container.name": "logging", }, }, gomock.Any()).Return("sidecarMockDigest2", nil) + m.mockLabeledTermPrinter.EXPECT().IsDone().Return(true).AnyTimes() + m.mockLabeledTermPrinter.EXPECT().Print().Return(nil).AnyTimes() m.mockAddons = nil }, wantImages: map[string]ContainerImageIdentifier{ @@ -593,10 +603,11 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { defer ctrl.Finish() m := &deployMocks{ - mockUploader: mocks.NewMockuploader(ctrl), - mockAddons: mocks.NewMockstackBuilder(ctrl), - mockRepositoryService: mocks.NewMockrepositoryService(ctrl), - mockFileSystem: afero.NewMemMapFs(), + mockUploader: mocks.NewMockuploader(ctrl), + mockAddons: mocks.NewMockstackBuilder(ctrl), + mockRepositoryService: mocks.NewMockrepositoryService(ctrl), + mockFileSystem: afero.NewMemMapFs(), + mockLabeledTermPrinter: mocks.NewMocklabeledTermPrinter(ctrl), } tc.mock(t, m) @@ -634,6 +645,9 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { templateFS: fakeTemplateFS(), overrider: new(override.Noop), customResources: crFn, + labeledTermPrinter: func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter { + return m.mockLabeledTermPrinter + }, } if m.mockAddons != nil { wkldDeployer.addons = m.mockAddons From 272650c82daa47f2aaa68144d3913c4672f5a521 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Mon, 17 Apr 2023 09:52:16 -0700 Subject: [PATCH 14/24] changes after rebase --- internal/pkg/cli/deploy/workload.go | 11 ++--------- internal/pkg/cli/deploy/workload_test.go | 1 - internal/pkg/cli/task_run_test.go | 2 +- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 0ec89b4216b..0686ad9a4c0 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -169,7 +169,6 @@ type workloadDeployer struct { mft interface{} rawMft []byte workspacePath string - uri string // Dependencies. fs afero.Fs @@ -253,10 +252,6 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { repoName := fmt.Sprintf("%s/%s", in.App.Name, in.Name) repository := repository.NewWithURI( ecr.New(defaultSessEnvRegion), repoName, resources.RepositoryURLs[in.Name]) - uri, err := repository.URI() - if err != nil { - return nil, fmt.Errorf("get ECR repository URI: %w", err) - } store := config.NewSSMStore(identity.New(defaultSession), ssm.New(defaultSession), aws.StringValue(defaultSession.Config.Region)) envDescriber, err := describe.NewEnvDescriber(describe.NewEnvDescriberConfig{ App: in.App.Name, @@ -285,7 +280,6 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { image: in.Image, resources: resources, workspacePath: ws.Path(), - uri: uri, fs: afero.NewOsFs(), s3Client: s3.New(envSession), addons: addons, @@ -387,7 +381,7 @@ func (img ContainerImageIdentifier) Tag() string { func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) error { // If it is built from local Dockerfile, build and push to the ECR repo. - buildArgsPerContainer, err := buildArgsPerContainer(d.name, d.workspacePath, d.uri, d.image, d.mft) + buildArgsPerContainer, err := buildArgsPerContainer(d.name, d.workspacePath, d.image, d.mft) if err != nil { return err } @@ -482,7 +476,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err return nil } -func buildArgsPerContainer(name, workspacePath, uri string, img ContainerImageIdentifier, unmarshaledManifest interface{}) (map[string]*dockerengine.BuildArguments, error) { +func buildArgsPerContainer(name, workspacePath string, img ContainerImageIdentifier, unmarshaledManifest interface{}) (map[string]*dockerengine.BuildArguments, error) { type dfArgs interface { BuildArgs(rootDirectory string) (map[string]*manifest.DockerBuildArgs, error) ContainerPlatform() string @@ -514,7 +508,6 @@ func buildArgsPerContainer(name, workspacePath, uri string, img ContainerImageId } labels[labelForContainerName] = container dArgs[container] = &dockerengine.BuildArguments{ - URI: uri, Dockerfile: aws.StringValue(buildArgs.Dockerfile), Context: aws.StringValue(buildArgs.Context), Args: buildArgs.Args, diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index f4f6239275f..9b21f45cedf 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -632,7 +632,6 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { GitShortCommitTag: tc.inMockGitTag, }, workspacePath: mockWorkspacePath, - uri: mockURI, mft: &mockWorkloadMft{ workloadName: mockName, fileName: tc.inEnvFile, diff --git a/internal/pkg/cli/task_run_test.go b/internal/pkg/cli/task_run_test.go index 37b47e9bce8..269393d4a09 100644 --- a/internal/pkg/cli/task_run_test.go +++ b/internal/pkg/cli/task_run_test.go @@ -860,7 +860,7 @@ func TestTaskRunOpts_Execute(t *testing.T) { Command: []string{}, EntryPoint: []string{}, }).Return(nil) - m.repository.EXPECT().Login().Return(nil) + m.repository.EXPECT().Login().Return(mockRepoURI, nil) m.repository.EXPECT().BuildAndPush(ctx, gomock.Any(), gomock.Any()) m.deployer.EXPECT().DeployTask(&deploy.CreateTaskResourcesInput{ Name: inGroupName, From 9a52d485d82946859147bf629e6b99d835c0d9a5 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Mon, 17 Apr 2023 11:42:46 -0700 Subject: [PATCH 15/24] addr Efe fb --- internal/pkg/cli/deploy/workload.go | 46 ++++++++++------------------- 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 0686ad9a4c0..02bb202c30e 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -63,8 +63,8 @@ const ( labelForContainerName = "com.aws.copilot.image.container.name" ) const ( - paddingForBuildAndPush = 5 - pollIntervalForBuildAndPush = 60 * time.Millisecond + paddingInSpacesForBuildAndPush = 5 + pollIntervalForBuildAndPush = 60 * time.Millisecond ) var ( @@ -393,18 +393,11 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err return fmt.Errorf("login to image repository: %w", err) } - var mu sync.Mutex + var digestsMu sync.Mutex out.ImageDigests = make(map[string]ContainerImageIdentifier, len(buildArgsPerContainer)) - - // counter for indexing the labeled output buffers. bufferIdx := 0 - // An array of buffers, one for each container image build and push output. labeledBuffers := make([]*syncbuffer.LabeledSyncBuffer, len(buildArgsPerContainer)) - - // create a context and an error group. g, ctx := errgroup.WithContext(context.Background()) - - // create a new cursor and hide it. cursor := cursor.New() cursor.Hide() @@ -413,12 +406,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err name := name buildArgs := buildArgs - // create a pipe for streaming the build and push output. - pr, pw := io.Pipe() - buildArgs.URI = uri - - // generate build args slice for docker build label. buildArgsList, err := buildArgs.GenerateDockerBuildArgs(dockerengine.New(exec.NewCmd())) if err != nil { return fmt.Errorf("generate docker build args of image %s: %w", name, err) @@ -427,38 +415,40 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err labeledBuffer := buf.WithLabel(fmt.Sprintf("Building your container image %s: docker %s", name, strings.Join(buildArgsList, " "))) labeledBuffers[bufferIdx] = labeledBuffer bufferIdx++ - + pr, pw := io.Pipe() g.Go(func() error { defer pw.Close() digest, err := d.repository.BuildAndPush(ctx, buildArgs, pw) if err != nil { return fmt.Errorf("build and push image %s: %w", name, err) } - mu.Lock() + digestsMu.Lock() out.ImageDigests[name] = ContainerImageIdentifier{ Digest: digest, CustomTag: d.image.CustomTag, GitShortCommitTag: d.image.GitShortCommitTag, } - mu.Unlock() + digestsMu.Unlock() return nil }) g.Go(func() error { if err := buf.Copy(pr); err != nil { - return fmt.Errorf("copying build and push output for container %s: %w", name, err) + return fmt.Errorf("copying build and push output for container image %s: %w", name, err) } return nil }) } - if isCIEnvironment() { - numLinesForBuildAndPush = -1 + opts := []syncbuffer.LabeledTermPrinterOption{syncbuffer.WithPadding(paddingInSpacesForBuildAndPush)} + if !isCIEnvironment() { + opts = append(opts, syncbuffer.WithNumLines(numLinesForBuildAndPush)) } - // create a LabeledTermPrinter for rendering build and push output. - ltp := d.labeledTermPrinter(os.Stderr, labeledBuffers, syncbuffer.WithNumLines(numLinesForBuildAndPush), syncbuffer.WithPadding(paddingForBuildAndPush)) - + ltp := d.labeledTermPrinter(os.Stderr, labeledBuffers, opts...) g.Go(func() error { for { if ltp.IsDone() { + if err := ltp.Print(); err != nil { + return fmt.Errorf("printing logs of docker build and push: %w", err) + } break } if err := ltp.Print(); err != nil { @@ -468,8 +458,6 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err } return nil }) - - // wait for all goroutines to complete and return any errors. if err := g.Wait(); err != nil { return err } @@ -524,10 +512,8 @@ func buildArgsPerContainer(name, workspacePath string, img ContainerImageIdentif // isCIEnvironment checks if the current environment is a continuous integration (CI) system. // Returns true by looking for the "CI" environment variable if it's set to "true", otherwise false. func isCIEnvironment() bool { - if ci, _ := os.LookupEnv("CI"); ci == "true" { - return true - } - return false + ci, _ := os.LookupEnv("CI") + return ci == "true" } func (d *workloadDeployer) uploadArtifactsToS3(out *UploadArtifactsOutput) error { From 9adf16b161388424f066ca64874ee531d1bf12e7 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Mon, 17 Apr 2023 12:55:57 -0700 Subject: [PATCH 16/24] change logic of if block --- internal/pkg/cli/deploy/workload.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 02bb202c30e..475d7b7b854 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -445,15 +445,12 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err ltp := d.labeledTermPrinter(os.Stderr, labeledBuffers, opts...) g.Go(func() error { for { - if ltp.IsDone() { - if err := ltp.Print(); err != nil { - return fmt.Errorf("printing logs of docker build and push: %w", err) - } - break - } if err := ltp.Print(); err != nil { return fmt.Errorf("printing logs of docker build and push: %w", err) } + if ltp.IsDone() { + break + } time.Sleep(pollIntervalForBuildAndPush) } return nil From 2cf1bdafa4962c3d709457712458fa0fc506907d Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Mon, 17 Apr 2023 17:07:23 -0700 Subject: [PATCH 17/24] addrWx fb: change error stmt and declare numLines as const add logic to termprinter for removing the buffer once they are done --- internal/pkg/cli/deploy/workload.go | 22 +++++++++------------ internal/pkg/repository/repository_test.go | 1 + internal/pkg/term/syncbuffer/termprinter.go | 2 ++ 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 475d7b7b854..caa9f79873f 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -65,10 +65,7 @@ const ( const ( paddingInSpacesForBuildAndPush = 5 pollIntervalForBuildAndPush = 60 * time.Millisecond -) - -var ( - numLinesForBuildAndPush = 5 + defaultNumLinesForBuildAndPush = 5 ) // ActionRecommender contains methods that output action recommendation. @@ -158,8 +155,6 @@ type GenerateCloudFormationTemplateOutput struct { Parameters string } -type labeledTermPrinterFunc func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter - type workloadDeployer struct { name string app *config.Application @@ -183,7 +178,7 @@ type workloadDeployer struct { envVersionGetter versionGetter overrider Overrider customResources customResourcesFunc - labeledTermPrinter labeledTermPrinterFunc + labeledTermPrinter func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter // Cached variables. defaultSess *session.Session @@ -273,6 +268,9 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { cfn := cloudformation.New(envSession, cloudformation.WithProgressTracker(os.Stderr)) + labeledTermPrinter := func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter { + return syncbuffer.NewLabeledTermPrinter(fw, bufs, opts...) + } return &workloadDeployer{ name: in.Name, app: in.App, @@ -297,9 +295,7 @@ func newWorkloadDeployer(in *WorkloadDeployerInput) (*workloadDeployer, error) { envSess: envSession, store: store, envConfig: envConfig, - labeledTermPrinter: func(fw syncbuffer.FileWriter, bufs []*syncbuffer.LabeledSyncBuffer, opts ...syncbuffer.LabeledTermPrinterOption) labeledTermPrinter { - return syncbuffer.NewLabeledTermPrinter(fw, bufs, opts...) - }, + labeledTermPrinter: labeledTermPrinter, mft: in.Mft, rawMft: in.RawMft, @@ -409,7 +405,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err buildArgs.URI = uri buildArgsList, err := buildArgs.GenerateDockerBuildArgs(dockerengine.New(exec.NewCmd())) if err != nil { - return fmt.Errorf("generate docker build args of image %s: %w", name, err) + return fmt.Errorf("generate docker build args for %q's image: %w", name, err) } buf := syncbuffer.New() labeledBuffer := buf.WithLabel(fmt.Sprintf("Building your container image %s: docker %s", name, strings.Join(buildArgsList, " "))) @@ -420,7 +416,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err defer pw.Close() digest, err := d.repository.BuildAndPush(ctx, buildArgs, pw) if err != nil { - return fmt.Errorf("build and push image %s: %w", name, err) + return fmt.Errorf("build and push the image for %q container: %w", name, err) } digestsMu.Lock() out.ImageDigests[name] = ContainerImageIdentifier{ @@ -440,7 +436,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err } opts := []syncbuffer.LabeledTermPrinterOption{syncbuffer.WithPadding(paddingInSpacesForBuildAndPush)} if !isCIEnvironment() { - opts = append(opts, syncbuffer.WithNumLines(numLinesForBuildAndPush)) + opts = append(opts, syncbuffer.WithNumLines(defaultNumLinesForBuildAndPush)) } ltp := d.labeledTermPrinter(os.Stderr, labeledBuffers, opts...) g.Go(func() error { diff --git a/internal/pkg/repository/repository_test.go b/internal/pkg/repository/repository_test.go index 0d86abe2545..fd409200791 100644 --- a/internal/pkg/repository/repository_test.go +++ b/internal/pkg/repository/repository_test.go @@ -56,6 +56,7 @@ func TestRepository_BuildAndPush(t *testing.T) { }, inMockDocker: func(m *mocks.MockContainerLoginBuildPusher) { m.EXPECT().Build(ctx, &defaultDockerArguments, gomock.Any()).Return(errors.New("error building image")) + m.EXPECT().Push(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) }, wantedError: fmt.Errorf("build Dockerfile at %s: error building image", inDockerfilePath), }, diff --git a/internal/pkg/term/syncbuffer/termprinter.go b/internal/pkg/term/syncbuffer/termprinter.go index aa8581ed3d0..00a5e3ac7a2 100644 --- a/internal/pkg/term/syncbuffer/termprinter.go +++ b/internal/pkg/term/syncbuffer/termprinter.go @@ -116,6 +116,8 @@ func (ltp *LabeledTermPrinter) printAll() error { if _, err := ltp.writeLines(buf.label, outputLogs); err != nil { return fmt.Errorf("write logs: %w", err) } + ltp.buffers = append(ltp.buffers[:idx], ltp.buffers[idx+1:]...) + idx-- } return nil } From 397b04b53851e46d868ed38796a93824bbc6725a Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Mon, 17 Apr 2023 18:23:41 -0700 Subject: [PATCH 18/24] fix tests --- internal/pkg/term/syncbuffer/termprinter.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/pkg/term/syncbuffer/termprinter.go b/internal/pkg/term/syncbuffer/termprinter.go index 00a5e3ac7a2..3b9db6c23a7 100644 --- a/internal/pkg/term/syncbuffer/termprinter.go +++ b/internal/pkg/term/syncbuffer/termprinter.go @@ -108,12 +108,12 @@ func (ltp *LabeledTermPrinter) Print() error { // If one of the buffer gets done then print entire content of the buffer. // Until all the buffers are written to file writer. func (ltp *LabeledTermPrinter) printAll() error { - for idx, buf := range ltp.buffers { - if !buf.IsDone() { + for idx := 0; idx < len(ltp.buffers); idx++ { + if !ltp.buffers[idx].IsDone() { continue } outputLogs := ltp.buffers[idx].lines() - if _, err := ltp.writeLines(buf.label, outputLogs); err != nil { + if _, err := ltp.writeLines(ltp.buffers[idx].label, outputLogs); err != nil { return fmt.Errorf("write logs: %w", err) } ltp.buffers = append(ltp.buffers[:idx], ltp.buffers[idx+1:]...) From 97393fc557ec64489a930e56c16410bf59d75354 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Mon, 17 Apr 2023 18:47:23 -0700 Subject: [PATCH 19/24] fix one more test --- internal/pkg/cli/deploy/workload.go | 2 +- internal/pkg/cli/deploy/workload_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index caa9f79873f..707213bb4f1 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -429,7 +429,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err }) g.Go(func() error { if err := buf.Copy(pr); err != nil { - return fmt.Errorf("copying build and push output for container image %s: %w", name, err) + return fmt.Errorf("copying build and push output for %q container: %w", name, err) } return nil }) diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index 9b21f45cedf..71da06232b5 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -220,7 +220,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { m.mockLabeledTermPrinter.EXPECT().IsDone().Return(true).AnyTimes() m.mockLabeledTermPrinter.EXPECT().Print().Return(nil).AnyTimes() }, - wantErr: fmt.Errorf("build and push image mockWkld: some error"), + wantErr: fmt.Errorf("build and push the image for \"mockWkld\" container: some error"), }, "build and push image with usertag successfully": { inMockUserTag: "v1.0", From 068d43f20f7d5ef2a528b810f918b229811a44a9 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 18 Apr 2023 09:36:14 -0700 Subject: [PATCH 20/24] addr fb: add multiples to printall to check for buffers printing only once --- internal/pkg/term/syncbuffer/termprinter_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/pkg/term/syncbuffer/termprinter_test.go b/internal/pkg/term/syncbuffer/termprinter_test.go index f761d950d31..19ffad0b6c3 100644 --- a/internal/pkg/term/syncbuffer/termprinter_test.go +++ b/internal/pkg/term/syncbuffer/termprinter_test.go @@ -90,6 +90,11 @@ line4 from image2`) buf.MarkDone() } ltp.Print() + if ltp.numLines == -1 { + for i := 0; i < 3; i++ { + ltp.Print() + } + } // THEN require.Equal(t, tc.wanted, termOut.String()) From 4cd0f33fcfe2bc4a874d6ab8d58a678a1a03a5e6 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 18 Apr 2023 09:44:49 -0700 Subject: [PATCH 21/24] add printAll --- internal/pkg/term/syncbuffer/termprinter_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/pkg/term/syncbuffer/termprinter_test.go b/internal/pkg/term/syncbuffer/termprinter_test.go index 19ffad0b6c3..eb372db5f3b 100644 --- a/internal/pkg/term/syncbuffer/termprinter_test.go +++ b/internal/pkg/term/syncbuffer/termprinter_test.go @@ -23,6 +23,7 @@ func TestLabeledTermPrinter_Print(t *testing.T) { testCases := map[string]struct { inNumLines int inPadding int + printAll bool wanted string }{ "display label with given numLines": { @@ -40,6 +41,7 @@ Building your container image 2 "display all the lines if numLines set to -1": { inNumLines: -1, inPadding: 5, + printAll: true, wanted: `Building your container image 1 line1 from image1 line2 from image1 @@ -90,7 +92,7 @@ line4 from image2`) buf.MarkDone() } ltp.Print() - if ltp.numLines == -1 { + if tc.printAll { for i := 0; i < 3; i++ { ltp.Print() } From f0c98536a261151c87eaf1a199a4854b63ff83cc Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 18 Apr 2023 09:50:17 -0700 Subject: [PATCH 22/24] add comment --- internal/pkg/term/syncbuffer/termprinter_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/pkg/term/syncbuffer/termprinter_test.go b/internal/pkg/term/syncbuffer/termprinter_test.go index eb372db5f3b..9cd5340e386 100644 --- a/internal/pkg/term/syncbuffer/termprinter_test.go +++ b/internal/pkg/term/syncbuffer/termprinter_test.go @@ -92,6 +92,9 @@ line4 from image2`) buf.MarkDone() } ltp.Print() + + // checking multiple calls to Print will result in + // printing a buffer only once when it enters printAll. if tc.printAll { for i := 0; i < 3; i++ { ltp.Print() From 28bb7624053eda2d73fabe81ce7d87f77c665f75 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 18 Apr 2023 14:50:45 -0700 Subject: [PATCH 23/24] addr @dannyrandall fb --- internal/pkg/cli/deploy/workload.go | 28 +++++++----------------- internal/pkg/cli/deploy/workload_test.go | 2 +- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 707213bb4f1..06a7cad9a34 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -391,12 +391,10 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err var digestsMu sync.Mutex out.ImageDigests = make(map[string]ContainerImageIdentifier, len(buildArgsPerContainer)) - bufferIdx := 0 - labeledBuffers := make([]*syncbuffer.LabeledSyncBuffer, len(buildArgsPerContainer)) + var labeledBuffers []*syncbuffer.LabeledSyncBuffer g, ctx := errgroup.WithContext(context.Background()) cursor := cursor.New() cursor.Hide() - for name, buildArgs := range buildArgsPerContainer { // create a copy of loop variables to avoid data race. name := name @@ -405,18 +403,16 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err buildArgs.URI = uri buildArgsList, err := buildArgs.GenerateDockerBuildArgs(dockerengine.New(exec.NewCmd())) if err != nil { - return fmt.Errorf("generate docker build args for %q's image: %w", name, err) + return fmt.Errorf("generate docker build args for %q: %w", name, err) } buf := syncbuffer.New() - labeledBuffer := buf.WithLabel(fmt.Sprintf("Building your container image %s: docker %s", name, strings.Join(buildArgsList, " "))) - labeledBuffers[bufferIdx] = labeledBuffer - bufferIdx++ + labeledBuffers = append(labeledBuffers, buf.WithLabel(fmt.Sprintf("Building your container image %q: docker %s", name, strings.Join(buildArgsList, " ")))) pr, pw := io.Pipe() g.Go(func() error { defer pw.Close() digest, err := d.repository.BuildAndPush(ctx, buildArgs, pw) if err != nil { - return fmt.Errorf("build and push the image for %q container: %w", name, err) + return fmt.Errorf("build and push the image %q: %w", name, err) } digestsMu.Lock() out.ImageDigests[name] = ContainerImageIdentifier{ @@ -429,27 +425,26 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err }) g.Go(func() error { if err := buf.Copy(pr); err != nil { - return fmt.Errorf("copying build and push output for %q container: %w", name, err) + return fmt.Errorf("copy build and push output for %q: %w", name, err) } return nil }) } opts := []syncbuffer.LabeledTermPrinterOption{syncbuffer.WithPadding(paddingInSpacesForBuildAndPush)} - if !isCIEnvironment() { + if os.Getenv("CI") != "true" { opts = append(opts, syncbuffer.WithNumLines(defaultNumLinesForBuildAndPush)) } ltp := d.labeledTermPrinter(os.Stderr, labeledBuffers, opts...) g.Go(func() error { for { if err := ltp.Print(); err != nil { - return fmt.Errorf("printing logs of docker build and push: %w", err) + return fmt.Errorf("print logs: %w", err) } if ltp.IsDone() { - break + return nil } time.Sleep(pollIntervalForBuildAndPush) } - return nil }) if err := g.Wait(); err != nil { return err @@ -502,13 +497,6 @@ func buildArgsPerContainer(name, workspacePath string, img ContainerImageIdentif return dArgs, nil } -// isCIEnvironment checks if the current environment is a continuous integration (CI) system. -// Returns true by looking for the "CI" environment variable if it's set to "true", otherwise false. -func isCIEnvironment() bool { - ci, _ := os.LookupEnv("CI") - return ci == "true" -} - func (d *workloadDeployer) uploadArtifactsToS3(out *UploadArtifactsOutput) error { var err error out.EnvFileARNs, err = d.pushEnvFilesToS3Bucket(&pushEnvFilesToS3BucketInput{ diff --git a/internal/pkg/cli/deploy/workload_test.go b/internal/pkg/cli/deploy/workload_test.go index 71da06232b5..8be0b333296 100644 --- a/internal/pkg/cli/deploy/workload_test.go +++ b/internal/pkg/cli/deploy/workload_test.go @@ -220,7 +220,7 @@ func TestWorkloadDeployer_UploadArtifacts(t *testing.T) { m.mockLabeledTermPrinter.EXPECT().IsDone().Return(true).AnyTimes() m.mockLabeledTermPrinter.EXPECT().Print().Return(nil).AnyTimes() }, - wantErr: fmt.Errorf("build and push the image for \"mockWkld\" container: some error"), + wantErr: fmt.Errorf("build and push the image \"mockWkld\": some error"), }, "build and push image with usertag successfully": { inMockUserTag: "v1.0", From bfd918c4b94a700a471495e002a96a4191068d5a Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 18 Apr 2023 16:07:08 -0700 Subject: [PATCH 24/24] use defer --- internal/pkg/cli/deploy/workload.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 06a7cad9a34..1cf3ae5fd97 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -415,12 +415,12 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err return fmt.Errorf("build and push the image %q: %w", name, err) } digestsMu.Lock() + defer digestsMu.Unlock() out.ImageDigests[name] = ContainerImageIdentifier{ Digest: digest, CustomTag: d.image.CustomTag, GitShortCommitTag: d.image.GitShortCommitTag, } - digestsMu.Unlock() return nil }) g.Go(func() error {