fix: Check to see if an application exists globally before creating it by KollaAdithya · Pull Request #4113 · aws/copilot-cli

KollaAdithya · 2022-10-26T16:05:44Z

This PR resolves #815
For example, if you run copilot app init my-proj in us-east-1, then run copilot app init my-proj in us-west-2 the second init will fail.
This happens because CloudFormation creates an adminrole for the application of the form {app-name}-adminrole. CloudFormation can't create two roles with the same name since roles are global.

Currently, CloudFormation errors out saying
failed to create: [AdministrationRole]
stack {app-name}-infrastructure-roles did not complete successfully and exited with status ROLLBACK_COMPLETE

This PR will do the following

Check to see if the application name exists in SSM
If it doesn't, check to see if the role exists
If the role exists, but the application doesn't exist shows an error: application named {app-name} already exists in another region
Case 1:
If you run copilot app init test-project in us-east-1 and If run copilot app init test-project in us-west-2 Then an error message shows application named test-project already exists in another region
Case 2:
If there is already a role existing in the account with same name as applicationname-adminrole then we will error out IAM admin role {applicationname}-adminrole already exists in this account

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

paragbhingre

Overall looks good 😎 I just have a question and a suggestion.

paragbhingre · 2022-10-26T16:35:13Z

Check to see if the application name exists in SSM

Are we actually checking anywhere that application does exist in the SSM?

KollaAdithya · 2022-10-26T17:00:33Z

There already a function func (s *Store) GetApplication(applicationName string) (*Application, error) that fetches the params in ssm by application name.
https://github.com/KollaAdithya/copilot-cli/blob/mainline/internal/pkg/config/app.go#L77

paragbhingre · 2022-10-26T20:15:08Z

There already a function func (s *Store) GetApplication(applicationName string) (*Application, error) that fetches the params in ssm by application name.
https://github.com/KollaAdithya/copilot-cli/blob/mainline/internal/pkg/config/app.go#L77

So just trying to understand the flow here. So currently we create admin-role and then add application to SSM? Is that why we are specifically checking if SSM doesn't have the application and admin-role exist before erroring out?

KollaAdithya · 2022-10-26T21:03:45Z

There already a function func (s *Store) GetApplication(applicationName string) (*Application, error) that fetches the params in ssm by application name.
https://github.com/KollaAdithya/copilot-cli/blob/mainline/internal/pkg/config/app.go#L77

So just trying to understand the flow here. So currently we create admin-role and then add application to SSM? Is that why we are specifically checking if SSM doesn't have the application and admin-role exist before erroring out?

Yes, when you are trying to deploy an application we create appname-adminrole and add application to Parameter Store. That is the reason we are checking SSM. So if the app name does not exist in SSM but appname-adminrole already exists means there is already an application with same name in other region. In this case we are erroring out that there is already an application with this name in another region.

codecov-commenter · 2022-10-26T23:05:06Z

Codecov Report

Merging #4113 (a1fb088) into mainline (0fec236) will increase coverage by 0.02%.
The diff coverage is 78.26%.

@@             Coverage Diff              @@
##           mainline    #4113      +/-   ##
============================================
+ Coverage     69.27%   69.29%   +0.02%     
============================================
  Files           250      250              
  Lines         35887    35919      +32     
  Branches        264      264              
============================================
+ Hits          24861    24891      +30     
- Misses         9834     9837       +3     
+ Partials       1192     1191       -1

Impacted Files	Coverage Δ
internal/pkg/cli/app_init.go	`64.16% <77.77%> (+3.62%)`	⬆️
internal/pkg/cli/validate.go	`73.77% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Lou1415926

Looks good! Just some nits around the recommendations!

github-actions · 2022-11-09T18:18:21Z

🍕 Here are the new binary sizes!

Name	New size (kiB)	v1.23.0 size (kiB)	Delta (%)
macOS (amd)	47572	47548	+0.05
macOS (arm)	47572	48200	❤️ -1.30
linux (amd)	41836	41812	+0.06
linux (arm)	41836	41220	🥺 +1.49
windows (amd)	38684	38664	+0.05

dannyrandall

lgtm!

Lou1415926

The tests LGTM but why is the coverage dropping 😭

aws#4113) This PR resolves aws#815 For example, if you run `copilot app init my-proj` in `us-east-1`, then run `copilot app init my-proj` in `us-west-2` the second init will fail. This happens because CloudFormation creates an adminrole for the application of the form `{app-name}-adminrole`. CloudFormation can't create two roles with the same name since roles are global. Currently, CloudFormation errors out saying failed to create: [AdministrationRole] `stack {app-name}-infrastructure-roles did not complete successfully and exited with status ROLLBACK_COMPLETE` This PR will do the following 1. Check to see if the application name exists in SSM 2. If it doesn't, check to see if the role exists 3. If the role exists, but the application doesn't exist shows an error: `application named {app-name} already exists in another region` Case 1: If you run `copilot app init test-project` in `us-east-1` and If run `copilot app init test-project `in `us-west-2` Then an error message shows `application named test-project already exists in another region` Case 2: If there is already a role existing in the account with same name as `applicationname-adminrole` then we will error out `IAM admin role {applicationname}-adminrole already exists in this account` By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

checking appname globally

9ca43cb

KollaAdithya requested a review from a team as a code owner October 26, 2022 16:05

KollaAdithya requested review from efekarakus and removed request for a team October 26, 2022 16:05

amazon-ecs-cli-v2-pr-manager requested a review from Lou1415926 October 26, 2022 16:05

paragbhingre reviewed Oct 26, 2022

View reviewed changes

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

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

address some fb

13bb92f

Lou1415926 reviewed Oct 26, 2022

View reviewed changes

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

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

address @Lou1415926 fb

bc28cce

revert unnecessary changes

f4f9e8f

Lou1415926 reviewed Oct 27, 2022

View reviewed changes

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

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

KollaAdithya added 3 commits October 28, 2022 14:35

addreess @ Lou1415926 feedback

3d5d728

add testcase

b333f7d

remove extra spaces

0e42d2b

Lou1415926 reviewed Oct 28, 2022

View reviewed changes

KollaAdithya added 2 commits October 28, 2022 19:09

address some feedback

732c80d

address fb

97677c9

Lou1415926 reviewed Oct 31, 2022

View reviewed changes

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

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

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

address some fb

69bf8e8

dannyrandall added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Oct 31, 2022

KollaAdithya added 2 commits October 31, 2022 16:25

fix test cases

9536175

additional scenario

a068732

huanjani removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 1, 2022

dannyrandall reviewed Nov 8, 2022

View reviewed changes

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

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

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

Comment thread internal/pkg/cli/app_init.go

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

iamhopaul123 reviewed Nov 8, 2022

View reviewed changes

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

address @dannyrandall fb

c007701

dannyrandall approved these changes Nov 9, 2022

View reviewed changes

address danny's fb and fix integ tests

6e0c16d

Lou1415926 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Nov 11, 2022

Lou1415926 approved these changes Nov 11, 2022

View reviewed changes

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

Comment thread internal/pkg/cli/app_init.go

KollaAdithya added 2 commits November 11, 2022 14:40

change function name

45a0470

resolving conflicts

a1fb088

KollaAdithya removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 14, 2022

mergify Bot merged commit 4b5837e into aws:mainline Nov 14, 2022

KollaAdithya deleted the appglobal branch February 7, 2023 01:59

Conversation

KollaAdithya commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paragbhingre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

paragbhingre commented Oct 26, 2022

Uh oh!

KollaAdithya commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paragbhingre commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KollaAdithya commented Oct 26, 2022

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dannyrandall left a comment

Choose a reason for hiding this comment

Uh oh!

Lou1415926 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

KollaAdithya commented Oct 26, 2022 •

edited

Loading

KollaAdithya commented Oct 26, 2022 •

edited

Loading

paragbhingre commented Oct 26, 2022 •

edited

Loading

codecov-commenter commented Oct 26, 2022 •

edited

Loading

github-actions Bot commented Nov 9, 2022 •

edited

Loading