chore(ecs): describe job and generate run task request from job workload by Lou1415926 · Pull Request #2246 · aws/copilot-cli

Lou1415926 · 2021-04-30T19:38:59Z

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.

huanjani · 2021-05-03T20:52:00Z

-	taskImageTagFlagDescription = `Optional. The container image tag in addition to "latest".`
+	taskImageTagFlagDescription    = `Optional. The container image tag in addition to "latest".`
+	generateCommandFlagDescription = `Optional. Generate a command with a pre-filled value for each flag.
+To use a non-Copilot ECS service, specify --generate-cmd <cluster name>/<service name>.


Q: we have fully surfaced the term "workload" to users at this point, right? If so, use "workload" here and in the next line instead of "service"?

Good catch. I should update the flag description in this PR. I'd refrain from using the keyword workload because it doesn't appear anywhere formally in our doc site. I will just say "service or job".

The changes to flag.go in this PR is a result of #2201. I will rebase this branch to mainline once that one is merged.
The real changes in this PR are located at ecs/run_task_request.go and aws/stepfunctions.

huanjani · 2021-05-03T20:53:51Z

+			return nil, fmt.Errorf("generate task run command from service: %w", err)
+		}
+	default:
+		return nil, errors.New("invalid input to --generate-cmd: must be of the form <cluster>/<service> or <app>/<env>/<workload>")


same as above: "workload" instead of "service" for the non-Copilot part of the error message?

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

@efekarakus

This PR is a continued work from aws#2225 by @efekarakus, and adds localized docs for Japanese language in our docs 🚀 Note that the localized docs are translated from the v1.6 Copilot docs - https://github.com/aws/copilot-cli/tree/v1.6.0/site, and are not from the mainline branch.  By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.11.0 to 1.12.0. - [Release notes](https://github.com/onsi/gomega/releases) - [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md) - [Commits](onsi/gomega@v1.11.0...v1.12.0) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.38.30 to 1.38.36. - [Release notes](https://github.com/aws/aws-sdk-go/releases) - [Changelog](https://github.com/aws/aws-sdk-go/blob/main/CHANGELOG.md) - [Commits](aws/aws-sdk-go@v1.38.30...v1.38.36) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

bvtujo

This is looking really good! Some nits about the custom unmarshaler for that deeply nested struct, however!

bvtujo · 2021-04-30T22:58:01Z

+	taskImageTagFlagDescription    = `Optional. The container image tag in addition to "latest".`
+	generateCommandFlagDescription = `Optional. Generate a command with a pre-filled value for each flag.
+To use a non-Copilot ECS service, specify --generate-cmd <cluster name>/<service name>.
+To use a Copilot service, specify --generate-cmd <application>/<environment>/<service>.


nit: can we update this language to include jobs?

Will address in next PR!

bvtujo · 2021-04-30T23:40:39Z

+
+type NetworkConfiguration ecs.NetworkConfiguration
+
+func (n *NetworkConfiguration) UnmarshalJSON(b []byte) error {


Can we edit this comment to make sure that it clarifies what kind of byte stream we expect? Because this unmarshaling logic is quite deeply nested we should be careful to clarify that this unmarshals just the network configuration from inside the DescribeStateMachineDefinition response.

bvtujo · 2021-05-10T16:26:10Z

-	ecsClient ecsClient
+	rgGetter       resourceGetter
+	ecsClient      ecsClient
+	StepFuncClient stepFunctionsClient


Does this client need to be exported?

They don't since they will be initialized by New(sess) so that the caller do not need to initialize these fields individually across package.

Although I think we could probably refactor this such that the clients are initialized from the caller (seems like a better approach to me) - in which case they will need to be exported.

huanjani

Looks good!

…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 enables generating a task run command from a Copilot job. Manually tested. Preceding PR: #2246 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 enables generating a task run command from a Copilot job. Manually tested. Preceding PR: aws#2246 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

amazon-ecs-cli-v2-pr-manager requested a review from bvtujo April 30, 2021 19:39

huanjani reviewed May 3, 2021

View reviewed changes

Lou1415926 added 6 commits May 7, 2021 11:40

add stepmachine client

fc4733d

describe job neetwork config

38ca113

run task request from job

14442f6

implement custom unmarshaling to avoid deeply nested struct

e282a61

fix lint

fa00abb

add test

d92579e

Lou1415926 force-pushed the generate/job branch from 7a2253b to d92579e Compare May 7, 2021 17:44

Lou1415926 and others added 2 commits May 7, 2021 12:47

add public comment

9adce70

chore: implement ask for secret init (aws#2271)

c585dfa

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

Lou1415926 marked this pull request as ready for review May 7, 2021 21:30

Lou1415926 requested a review from a team as a code owner May 7, 2021 21:30

toricls and others added 3 commits May 8, 2021 17:20

bvtujo reviewed May 10, 2021

View reviewed changes

bvtujo approved these changes May 10, 2021

View reviewed changes

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

huanjani approved these changes May 10, 2021

View reviewed changes

Lou1415926 added 9 commits May 10, 2021 16:31

add stepmachine client

d922b4a

describe job neetwork config

0cf3da5

run task request from job

269227b

implement custom unmarshaling to avoid deeply nested struct

921fcbf

fix lint

4139be3

add test

c77c0d2

add public comment

b5b30f0

update custom UnmarshalJSON comment

e464819

fix conflict

711b369

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

mergify Bot merged commit 904dd44 into aws:mainline May 10, 2021

Lou1415926 mentioned this pull request May 13, 2021

chore(cli): generate task run command from a Copilot job #2312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

chore(ecs): describe job and generate run task request from job workload#2246
mergify[bot] merged 20 commits intoaws:mainlinefrom
Lou1415926:generate/job

Lou1415926 commented Apr 30, 2021 •

edited

Loading

Uh oh!

huanjani May 3, 2021

Uh oh!

Lou1415926 May 3, 2021

Uh oh!

huanjani May 3, 2021

Uh oh!

bvtujo left a comment

Uh oh!

bvtujo Apr 30, 2021

Uh oh!

Lou1415926 May 10, 2021

Uh oh!

Uh oh!

bvtujo Apr 30, 2021

Uh oh!

Lou1415926 May 10, 2021

Uh oh!

bvtujo May 10, 2021

Uh oh!

Lou1415926 May 10, 2021

Uh oh!

huanjani left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants


		type NetworkConfiguration ecs.NetworkConfiguration

		func (n *NetworkConfiguration) UnmarshalJSON(b []byte) error {

Conversation

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

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!

bvtujo left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

huanjani left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Lou1415926 commented Apr 30, 2021 •

edited

Loading