feat(cli): implement exec for task run #1077

Louise15926 · 2020-06-30T21:06:15Z

The PR implements execute for task run command. Specifically, it does the following:

modifies ecs package, and adds a ec2 package
modifies deploy, cloudformation and stack packages
creates a cloudformation template for task run
modifies cli package

Related #702

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

iamhopaul123

Awesome PR. A great leap from 0 to 1!

internal/pkg/aws/ec2/ec2.go

internal/pkg/cli/task_run.go

iamhopaul123

Looks really good to me! Could you also add manual tests for this feature? Like doing a deployment with local image and url?

internal/pkg/aws/ec2/ec2.go

internal/pkg/cli/interfaces.go

internal/pkg/aws/ec2/ec2.go

internal/pkg/cli/task_run.go

templates/task/cf.yml

bvtujo

This looks awesome. What a massive effort. Just a few nits from my side but LGTM!

internal/pkg/aws/ecs/ecs.go

internal/pkg/deploy/cloudformation/stack/name.go

internal/pkg/deploy/task.go

internal/pkg/deploy/cloudformation/task.go

internal/pkg/deploy/cloudformation/stack/task.go

internal/pkg/cli/task_run.go

iamhopaul123

ship it just a few nits and adding the manual test

internal/pkg/aws/ec2/ec2.go

internal/pkg/aws/ecs/ecs.go

kohidave · 2020-07-07T21:02:01Z

internal/pkg/aws/ecs/ecs.go

@@ -168,6 +181,60 @@ func (e *ECS) ServiceTasks(clusterName, serviceName string) ([]*Task, error) {
 	return tasks, nil
 }

+// DefaultClusters returns the default clusters in the account and region
+func (e *ECS) DefaultCluster() (string, error) {
+	resp, err := e.client.DescribeClusters(&ecs.DescribeClustersInput{})


Shouldn't we include the "Default" cluster as an input parameter? Otherwise we're just choosing the "first" cluster - which may not be the default cluster at all.

From DescribeClustersInput it says, If Clusters is not specified, the default cluster is assumed. Not sure if this answers your concern 👀

internal/pkg/aws/ecs/ecs.go

kohidave · 2020-07-07T22:04:34Z

internal/pkg/cli/task_run.go

+	clusters, err := o.resourceGetter.GetResourcesByTags(clusterResourceType, map[string]string{
+		deploy.AppTagKey: o.AppName(),
+		deploy.EnvTagKey: o.env,
+	})


I think we should hide this somewhere. Perhaps in the describe package or the ECSGetter?

kohidave · 2020-07-07T22:14:08Z

internal/pkg/cli/task_run.go

-	store  store
-	parser dockerfileParser
-	sel    appEnvWithNoneSelector
+	fs    afero.Fs


Is this used?

It's used at Validate to determine whether the dockerfile path that user provides is valid

kohidave · 2020-07-07T22:18:33Z

internal/pkg/deploy/cloudformation/stack/task.go

+
+	Cpu    int
+	Memory int
+


We may also need to support EntryPoint, as well.

A new flag🚩 Sounds good! Will do this in a separate PR!

templates/task/cf.yml

internal/pkg/aws/ec2/ec2.go

internal/pkg/aws/ecs/ecs.go

internal/pkg/cli/interfaces.go

efekarakus

This is a huge amount of work and amazing! However, can we close this PR and create new ones by splitting up this work?

I think for us to be able to give good feedback, we need the PRs to be smaller. I personally struggle digesting all this information. We can take our time slightly more but deliver the right UX for our customers.

What do you think of submitting a PR individually for each one of the packages touched?

internal/aws/ec2 pkg
deploy/cfn/stack pkg + cf template
deploy/cfn pkg with DeployTask
An internal/task pkg with multiple functions like RunInDefaultVPC or RunInEnv that could
abstract away the getCluster, getNetworking methods.
An internal/repository pkg to abstract pushToECRRepo()
The task_run file.

Feel free to change the order or add additional/remove item but in general let's try to keep each layer in a separate PR.

Also this is my bad, I'll make sure to provide quick feedback with the PRs to unblock you!

internal/pkg/cli/task_run.go

efekarakus · 2020-07-08T16:02:20Z

internal/pkg/cli/task_run.go

+)
+
+const (
+	defaultImageTag       = "copilot"


Can we re-use:

copilot-cli/internal/pkg/cli/git.go

Lines 13 to 21 in 68929f7

func getVersionTag(runner runner) (string, error) {

var b bytes.Buffer

if err := runner.Run("git", []string{"describe", "--always"}, command.Stdout(&b)); err != nil {

return "", err

}

// NOTE: `git describe` output bytes includes a `\n` character, so we trim it out.

return strings.TrimSpace(b.String()), nil

do you think it makes sense if I first try to getVersionTag, if it fails (e.g. it is not a git repo), then I use this defaultImageTag?

I think what we do with svc deploy is first checking if we can getVersionTag, if that fails we prompt for a tag value

internal/pkg/cli/task_run.go

efekarakus · 2020-07-08T16:21:38Z

internal/pkg/cli/task_run.go

 	cmd.Flags().StringSliceVar(&vars.securityGroups, securityGroupsFlag, nil, securityGroupsFlagDescription)

 	cmd.Flags().StringToStringVar(&vars.envVars, envVarsFlag, nil, envVarsFlagDescription)
-	cmd.Flags().StringSliceVar(&vars.commands, commandsFlag, nil, commandsFlagDescription)
+	cmd.Flags().StringVar(&vars.command, commandFlag, "", commandFlagDescription)


In the style guide for the CLI we decided: https://github.com/aws/copilot-cli/blob/master/STYLE_GUIDE.md#flags

Flags are used when you need more than one required input of different type or if the inputs are optional.

Since the flag is optional and to keep it consistent with the other commands, I think we should have it behind a flag

efekarakus · 2020-07-08T16:23:47Z

internal/pkg/cli/task_run.go

+}
+
+func (o *runTaskOpts) getCluster() (string, error) {
+	if o.env == config.EnvNameNone {


I find the EnvNameNone a bit confusing, so as a customer do I have to explicitly write "None" to the -e flag for it to pick up the default cluster?

Why can't we use the default empty string "" to say that a user hasn't specified an environment?
I'd love for us to think if we can simplify this for our customers since the "None" environment doesn't exist in our other commands.

No, they don't explicitly specify "None". The first time "None" appears is when we ask for an env name during Ask. We list all the environments of the app, and append to the end an option called "None" which would result in the default cluster and subnets if selected.

We can use "" instead of "None". As a customer the experience would be the same - the "None" would still be a reserved keyword because it's till gonna be there during Ask, so that the user can opt to use the default cluster/subnet during Ask.

Gotcha! that's good to know :) ok then I think it's fine but maybe we can move the const out of the config pkg to the cli package itself. Since config pkg itself doesn't use the constant.

Sounds good!!

internal/pkg/cli/task_run.go

Louise15926 requested a review from a team as a code owner June 30, 2020 21:06

Louise15926 requested a review from kohidave June 30, 2020 21:06

amazon-ecs-cli-v2-pr-manager requested a review from bvtujo June 30, 2020 21:06

Louise15926 requested a review from piradeepk June 30, 2020 21:06

Louise15926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 30, 2020

piradeepk requested a review from iamhopaul123 June 30, 2020 21:08

iamhopaul123 reviewed Jun 30, 2020

View reviewed changes

Lou1415926 added 3 commits July 6, 2020 14:34

add task's deployment method in deploy package

57e65ab

add runtask methods to ecs client and add vpc methods to ec2 client

c8c7182

implement task run exec

3030e3e

Louise15926 force-pushed the master branch from 5965034 to 3030e3e Compare July 6, 2020 20:36

iamhopaul123 reviewed Jul 6, 2020

View reviewed changes

address feedback

2ee06fc

Louise15926 force-pushed the master branch from 58d7c58 to 2ee06fc Compare July 7, 2020 17:07

bvtujo approved these changes Jul 7, 2020

View reviewed changes

iamhopaul123 reviewed Jul 7, 2020

View reviewed changes

internal/pkg/cli/task_run.go Show resolved Hide resolved

iamhopaul123 reviewed Jul 7, 2020

View reviewed changes

internal/pkg/cli/task_run.go Show resolved Hide resolved

iamhopaul123 reviewed Jul 7, 2020

View reviewed changes

internal/pkg/cli/task_run.go Show resolved Hide resolved

iamhopaul123 approved these changes Jul 7, 2020

View reviewed changes

kohidave reviewed Jul 7, 2020

View reviewed changes

efekarakus reviewed Jul 8, 2020

View reviewed changes

Louise15926 closed this Jul 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): implement exec for task run #1077

feat(cli): implement exec for task run #1077

Louise15926 commented Jun 30, 2020 •

edited

Loading

iamhopaul123 left a comment

iamhopaul123 left a comment

bvtujo left a comment

iamhopaul123 left a comment

kohidave Jul 7, 2020

Louise15926 Jul 7, 2020

kohidave Jul 7, 2020

kohidave Jul 7, 2020

Louise15926 Jul 7, 2020

kohidave Jul 7, 2020

Louise15926 Jul 8, 2020

efekarakus left a comment •

edited by Louise15926

Loading

efekarakus Jul 8, 2020

Louise15926 Jul 8, 2020

efekarakus Jul 8, 2020

efekarakus Jul 8, 2020

efekarakus Jul 8, 2020

Louise15926 Jul 8, 2020

efekarakus Jul 8, 2020

Louise15926 Jul 9, 2020

	func getVersionTag(runner runner) (string, error) {
	var b bytes.Buffer

	if err := runner.Run("git", []string{"describe", "--always"}, command.Stdout(&b)); err != nil {
	return "", err
	}

	// NOTE: `git describe` output bytes includes a `\n` character, so we trim it out.
	return strings.TrimSpace(b.String()), nil

feat(cli): implement exec for task run #1077

feat(cli): implement exec for task run #1077

Conversation

Louise15926 commented Jun 30, 2020 • edited Loading

iamhopaul123 left a comment

Choose a reason for hiding this comment

iamhopaul123 left a comment

Choose a reason for hiding this comment

bvtujo left a comment

Choose a reason for hiding this comment

iamhopaul123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efekarakus left a comment • edited by Louise15926 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Louise15926 commented Jun 30, 2020 •

edited

Loading

efekarakus left a comment •

edited by Louise15926

Loading