feat: implement --generate-cmd flag for task run by Lou1415926 · Pull Request #2201 · aws/copilot-cli

Lou1415926 · 2021-04-21T22:00:21Z

This PR implements --generate-cmd for task run given either a non-Copilot ECS service or a Copilot service.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Lou1415926 · 2021-04-21T22:28:00Z

Conducted the following manual testings.

For Copilot service

Deployed a Copilot service with an Aurora add-on (which requires correctly configured secret and security group for access)
Generate a command using the Copilot service. --generate-cmd <app>/<env>/<svc>
Run the generated command using the Copilot service

For non-Copilot ECS Service

Using cluster name and service name

Started an ECS service in default cluster with entrypoint and command. Didn't specify an execution role or task role.
Generate a command using the ECS service. --generate-cmd default/<service-name>
The
Run the generated command using the ECS service.
The task's task definition has the correct entrypoint/command override, image, subnet, security group, etc. It runs in default cluster. The execution role is the default execution role and no task role is assumed.

Using ARN

Generate a command using the same ECS service as above. --generate-cmd default/<service-name>
The
The command generated is the same as the command generated using cluster/service name.

… into generate/task-run

huanjani

Looks great!

bvtujo

Looks great to me! Just a few questions.

bvtujo · 2021-04-27T21:17:05Z

+	if err != nil {
+		return nil, fmt.Errorf("extract service name from arn %s", svcARN)
+	}
+	return generator.ECSServiceCommandGenerator{


Are we not supporting jobs with this feature? I apologize I didn't get in on the earlier review of this. Or was the decision made to delegate that to the hypothetical job run subcommand?

I will make a draft PR for job for this feature! The code is working but is a little messy and I haven't got the chance to really clean it up, so it's not in this PR yet.

efekarakus

The code looks good! let me know what's your thought on the comment below :)

efekarakus · 2021-04-28T17:23:43Z

+type cmdGenerator interface {
+	Generate() (*generator.GenerateCommandOpts, error)
+}


This is interesting 💭 this is the first time that we have a package that populates values for a command.

Did we go with this route to keep the cli pkg small? I think that makes sense to me, but I wonder if instead of a new pkg we could have moved most of the functionality into the internal/pkg/ecs pkg and removed the generator pkg altogether.
This functionality seems like should belong in thecli pkg since it can only be used by task_run so there is no other possible consumer of the generator pkg:

copilot-cli/internal/pkg/generator/generate.go

Lines 74 to 75 in f2ad676

func (o GenerateCommandOpts) String() string {

output := []string{"copilot task run"}

Hope that makes sense! Instead, we can maybe move this functionality to the ecs pkg:

copilot-cli/internal/pkg/generator/ecs_service.go

Line 29 in f2ad676

func (g ECSServiceCommandGenerator) Generate() (*GenerateCommandOpts, error) {

to something like:

func RunTaskRequestFromECSService(client ecsDescriber, cluster, service string) (*RunTaskRequest, error) func RunTaskRequestFromService(client ecsDescriber, app, env, svc string) (*RunTaskRequest, error)

iamhopaul123

Amazing work! LGTM mostly but just some nits.

iamhopaul123 · 2021-05-04T20:48:12Z

+	return nil
+}
+
+func (o *runTaskOpts) runTaskCommand() (*ecs.RunTaskRequest, error) {


Maybe i was missing anything but i don't see the logic is well tested for this func...

iamhopaul123 · 2021-05-04T20:59:05Z

+	}
+
+	if len(taskDef.ContainerDefinitions) > 1 {
+		return nil, fmt.Errorf("found more than one container in task definition: %s", taskDefNameOrARN)


I think for a better user experience, it would be nice if we can log here about the reason why they cannot use a taskdef with more than one container. But we don't log elsewhere than the cli pkg. What do you think of it @efekarakus ?

We can throw a typed error here and catch that in the cli package and log. So something like:

var errMultiContainers *ecs.ErrMultipleContainersInTaskDef if errors.As(err, &errMultiContainers) { log.Errorf("`copilot task run` does not support running more than one container") } return err

That's exactly what I've been thinking!

efekarakus

Yay looks great, just one more minor suggestion and then we can

efekarakus · 2021-05-05T16:22:25Z

+	}
+
+	if len(taskDef.ContainerDefinitions) > 1 {
+		return nil, fmt.Errorf("found more than one container in task definition: %s", taskDefNameOrARN)


We can throw a typed error here and catch that in the cli package and log. So something like:

var errMultiContainers *ecs.ErrMultipleContainersInTaskDef if errors.As(err, &errMultiContainers) { log.Errorf("`copilot task run` does not support running more than one container") } return err

efekarakus · 2021-05-05T16:32:08Z

+}
+
+// String stringifies a RunTaskRequest.
+func (r RunTaskRequest) String() string {


I don't think we should implement the fmt.Stringer interface if it can't be used across packages. For example, if we do fmt.Sprintf("%s", RunTaskRequest{...}) it's going to write "copilot task run ..." which is surprising.

Instead, we can create our own string method: CLIString() string and a type CLIStringer interface so that we can implement this interface across many data types

iamhopaul123

Nice tests! LGTM 🚢

…oad (#2246) This PR implement the function to generate a run task request from a deployed job, and the functions to support it. This PR is rebased from #2201 (which is not merged yet). The real changes to this PR are at - `ecs/run_task_request.go` (where the information needed to generated a `task run` command is grabbed from a Copilot job) - `ecs/run_task_request_test.go` - `aws/stepfunctions/*` (where the client to make API calls to stepfunctions is implemented) By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…oad (aws#2246) This PR implement the function to generate a run task request from a deployed job, and the functions to support it. This PR is rebased from aws#2201 (which is not merged yet). The real changes to this PR are at - `ecs/run_task_request.go` (where the information needed to generated a `task run` command is grabbed from a Copilot job) - `ecs/run_task_request_test.go` - `aws/stepfunctions/*` (where the client to make API calls to stepfunctions is implemented) By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This PR implements `--generate-cmd` for `task run` given either a non-Copilot ECS service or a Copilot service. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…oad (aws#2246) This PR implement the function to generate a run task request from a deployed job, and the functions to support it. This PR is rebased from aws#2201 (which is not merged yet). The real changes to this PR are at - `ecs/run_task_request.go` (where the information needed to generated a `task run` command is grabbed from a Copilot job) - `ecs/run_task_request_test.go` - `aws/stepfunctions/*` (where the client to make API calls to stepfunctions is implemented) By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Lou1415926 added 3 commits April 21, 2021 14:40

add generate-cmd flag to task run

c83ac79

update condition to prompt for app/env

3c24575

rearange code and add missing test

c5f08d5

Lou1415926 requested a review from a team as a code owner April 21, 2021 22:00

Lou1415926 requested a review from bvtujo April 21, 2021 22:00

amazon-ecs-cli-v2-pr-manager requested a review from huanjani April 21, 2021 22:00

fix unit test

8ca34f9

Merge branch 'mainline' into generate/task-run

b7a64aa

huanjani reviewed Apr 22, 2021

View reviewed changes

Comment thread internal/pkg/cli/flag.go Outdated

huanjani reviewed Apr 22, 2021

View reviewed changes

Comment thread internal/pkg/cli/flag.go Outdated

Lou1415926 added 2 commits April 22, 2021 12:39

address feedback to change var name and flag description

b90ada6

Merge branch 'generate/task-run' of github.com:Lou1415926/copilot-cli…

fb8c9f7

… into generate/task-run

huanjani reviewed Apr 22, 2021

View reviewed changes

Comment thread internal/pkg/cli/task_run.go

huanjani reviewed Apr 22, 2021

View reviewed changes

Comment thread internal/pkg/cli/task_run_test.go Outdated

huanjani approved these changes Apr 22, 2021

View reviewed changes

address feedback to change remaining var name

32d489c

bvtujo reviewed Apr 27, 2021

View reviewed changes

efekarakus reviewed Apr 28, 2021

View reviewed changes

efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 29, 2021

Lou1415926 added 3 commits April 29, 2021 15:39

refactor generator into ecs pkg

5027eb1

update task run

a17af88

update public function comment

522fac5

bvtujo removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 30, 2021

Lou1415926 added 3 commits April 30, 2021 11:04

move private function to bottom

c1a5603

addd file header

e1a3100

rename variable

3b387f3

Lou1415926 mentioned this pull request May 3, 2021

chore(ecs): describe job and generate run task request from job workload #2246

Merged

iamhopaul123 reviewed May 4, 2021

View reviewed changes

bulk address feedback on error messaging

191ea9e

implement unit test for runTaskCommand()

b2ae755

efekarakus reviewed May 5, 2021

View reviewed changes

Lou1415926 added 5 commits May 5, 2021 11:57

address feedback to improve flag message, error message, and comment

6d139c3

address feedback to improve logging message

f6dde4a

fix

d68b6f0

abstract away clistringer

b7c5417

Merge branch 'mainline' into generate/task-run

c61de75

Lou1415926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label May 5, 2021

iamhopaul123 approved these changes May 5, 2021

View reviewed changes

Lou1415926 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label May 5, 2021

Merge branch 'mainline' into generate/task-run

4699160

mergify Bot merged commit ff64cf1 into aws:mainline May 5, 2021

	func (o GenerateCommandOpts) String() string {
	output := []string{"copilot task run"}

Conversation

Lou1415926 commented Apr 21, 2021

Uh oh!

Lou1415926 commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For Copilot service

For non-Copilot ECS Service

Using cluster name and service name

Using ARN

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huanjani left a comment

Choose a reason for hiding this comment

Uh oh!

bvtujo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamhopaul123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efekarakus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamhopaul123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Lou1415926 commented Apr 21, 2021 •

edited

Loading