fix: Use PublicSubnet from stack to run task by ota42y · Pull Request #2024 · aws/copilot-cli

ota42y · 2021-03-06T03:04:22Z

Please read this Issue.
#2022

There was a structure called EnvironmentVPC that was not being used, so we added data to it and returned it when describing the Stack.
We also used this information to get PublicSubnet when executing the Task.

We can get SecurityGroup from the Stack, but we have not changed it because it is not relevant to this change.

Lou1415926

Hello ota42y! Thank you so much for reporting and fixing the issue 🙇‍♀️ Really appreciated! I have some small suggestions below.

Co-authored-by: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com>

ota42y · 2021-03-09T10:37:29Z

I have completed the fix!

bvtujo

Thank you so much for this change, this will be super helpful. A few comments, nothing major :)

bvtujo · 2021-03-09T22:51:50Z

-	return tags, nil
+
+	for _, out := range envStack.Outputs {
+		if out.OutputKey != nil {


Can we use the aws.StringValue() function here to do this comparison? We tend to prefer it to explicit pointer dereference in this codebase. It could be coupled with a switch here as well:

for _, out := range envStack.Outputs { switch aws.StringValue(out.OutputKey) { case stack.EnvOutputVPCID: environmentVPC.ID = aws.StringValue(out.OutputValue) ... } }

We already import it into the describe package in other files, so it isn't adding a dependency, and it can avoid the extra conditional.

bvtujo · 2021-03-09T22:54:31Z

 				Environment: testEnv,
 				Services:    envSvcs,
 				Tags:        map[string]string{"copilot-application": "testApp", "copilot-environment": "testEnv"},
+				EnvironmentVPC: &EnvironmentVPC{


🙏 thank you so much for these tests!

bvtujo · 2021-03-09T22:58:19Z

 	}
-	if len(subnets) == 0 {
+	if description == nil || description.EnvironmentVPC == nil || len(description.EnvironmentVPC.PublicSubnetIDs) == 0 {
 		return nil, errNoSubnetFound


Can we use a more descriptive error here, since this is wrapping multiple failure modes? Perhaps we could break it out into separate descriptive errors.

I think we don't need to check description.
Because when description is nil, we'll get error and return above line.

And I don't use pointer for description.EnvironmentVPC .
Because when we can't get data from Stack, we return error so we can set this struct always.

Got it! Thanks for the follow up!

Co-authored-by: Austin Ely <austiely@amazon.com>

When we can't get Stack we'll get error, so this struct always exist.

ota42y · 2021-03-10T04:33:12Z

I have completed the fix!

Lou1415926

Thank you so much for the quick turnaround!!

bvtujo · 2021-03-10T17:04:21Z

Thank you so much for this, @ota42y! A super helpful enhancement.

This PR is a quick follow up on #2024. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Please read this Issue. aws#2022 There was a structure called EnvironmentVPC that was not being used, so we added data to it and returned it when describing the Stack. We also used this information to get PublicSubnet when executing the Task. We can get SecurityGroup from the Stack, but we have not changed it because it is not relevant to this change.

This PR is a quick follow up on aws#2024. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ota42y requested a review from a team as a code owner March 6, 2021 03:04

ota42y requested a review from bvtujo March 6, 2021 03:04

amazon-ecs-cli-v2-pr-manager requested a review from efekarakus March 6, 2021 03:04

ota42y mentioned this pull request Mar 6, 2021

Can't run task on environment which imported subnets #2022

Open

ota42y changed the title ~~Use PublicSubnet from stack to run task~~ Bug Fixes: Use PublicSubnet from stack to run task Mar 6, 2021

ota42y added 2 commits March 6, 2021 12:09

feat: load VPC environment form stack outputs

ac0168d

fix: get public subnets from stack outputs

60edf43

ota42y changed the title ~~Bug Fixes: Use PublicSubnet from stack to run task~~ fix: Use PublicSubnet from stack to run task Mar 6, 2021

ota42y force-pushed the feature/use_subnets_from_stack branch from 30d50f5 to 60edf43 Compare March 6, 2021 03:13

Lou1415926 reviewed Mar 8, 2021

View reviewed changes

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

Comment thread internal/pkg/describe/env.go Outdated

Comment thread internal/pkg/describe/env.go Outdated

Comment thread internal/pkg/task/env_runner.go Outdated

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

ota42y and others added 7 commits March 9, 2021 18:51

Update internal/pkg/cli/task_run.go

11bf888

Co-authored-by: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com>

Update internal/pkg/describe/env.go

0d8d31f

Co-authored-by: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com>

Update internal/pkg/cli/task_run.go

73156a3

Co-authored-by: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com>

Update internal/pkg/task/env_runner.go

59743f9

Co-authored-by: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com>

use fmtErrDescribeEnvironment

8a868c6

use Output name in internal/pkg/deploy/cloudformation/stack/env.go

925fa23

fix error message in test

d169ce0

bvtujo reviewed Mar 9, 2021

View reviewed changes

ota42y and others added 2 commits March 10, 2021 11:54

Update internal/pkg/cli/task_run.go

5a411a0

Co-authored-by: Austin Ely <austiely@amazon.com>

use aws.StringValue to check Stack data

483ff58

ota42y force-pushed the feature/use_subnets_from_stack branch 3 times, most recently from 3ef4c6b to 0377970 Compare March 10, 2021 04:15

don't use pointer for EnvironmentVPC

393e294

When we can't get Stack we'll get error, so this struct always exist.

ota42y force-pushed the feature/use_subnets_from_stack branch from 0377970 to 393e294 Compare March 10, 2021 04:23

ota42y mentioned this pull request Mar 10, 2021

chore: improve debugging for no subnets error in task run #2023

Closed

bvtujo approved these changes Mar 10, 2021

View reviewed changes

Lou1415926 approved these changes Mar 10, 2021

View reviewed changes

Merge branch 'mainline' into feature/use_subnets_from_stack

0db2968

mergify Bot merged commit 394cc1b into aws:mainline Mar 10, 2021

Lou1415926 mentioned this pull request Mar 10, 2021

chore(describe): encode json using camelCase #2037

Merged

ota42y deleted the feature/use_subnets_from_stack branch March 11, 2021 04:50

Conversation

ota42y commented Mar 6, 2021

Uh oh!

Lou1415926 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ota42y commented Mar 9, 2021

Uh oh!

bvtujo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bvtujo Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

bvtujo Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

bvtujo Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ota42y Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

bvtujo Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

ota42y commented Mar 10, 2021

Uh oh!

Lou1415926 left a comment

Choose a reason for hiding this comment

Uh oh!

bvtujo commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants