Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): make env init idempotent #763

Merged
merged 2 commits into from Mar 20, 2020

Conversation

iamhopaul123
Copy link
Contributor

Fix #301, partially addressing #660.

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

@iamhopaul123 iamhopaul123 added this to In review in Sprint 🏃‍♀️ via automation Mar 19, 2020
@iamhopaul123
Copy link
Contributor Author

iamhopaul123 commented Mar 19, 2020

Manual Test

Suppose we already have environment "test" set up and accidentally deleted it from SSM. Then if I run ecs-preview env init --env test again, it is supposed to get "test" env registered in SSM store.

$ ecs-preview env ls
[no env]
$ ecs-preview env init
? What is your environment's name? test

? Which named profile should we use to create test? default

✔ CloudFormation stack for env test already exists under project my-project! Do nothing.
✔ Linked account 1234567890 and region us-west-2 project my-project.
✔ Created environment test in region us-west-2 under project my-project.
$ ecs-preview env ls
test

Also tested if remove the CFN stack.

$ ecs-preview env init

? What is your environment's name? test

? Which named profile should we use to create test? default
✔ Created the infrastructure for the test environment.
- Virtual private cloud on 2 availability zones to hold your services     [Complete]
  - Internet gateway to connect the network to the internet               [Complete]
  - Public subnets for internet facing services                           [Complete]
  - Private subnets for services that can't be reached from the internet  [Complete]
  - Routing tables for services to talk with each other                   [Complete]
- ECS Cluster to hold your services                                       [Complete]
- Application load balancer to distribute traffic                         [Complete]
✔ Linked account 1234567890 and region us-west-2 project my-project.
✔ Created environment test in region us-west-2 under project my-project.

Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you! A suggestion below

internal/pkg/store/env.go Show resolved Hide resolved
internal/pkg/store/store_integration_test.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/env.go Outdated Show resolved Hide resolved
Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Contributor

@kohidave kohidave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! One question about projects, though.

@@ -135,11 +134,7 @@ func (o *initProjectOpts) Execute() error {
Domain: o.DomainName,
})
if err != nil {
// If the project already exists, move on - otherwise return the error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this is interesting. Anyway, no matter what we do - we should move the creation of the project in SSM to after all the CloudFormation stack creation/updating.

Same with the workspace file writing.

I think up here, we should then check to see if the project already exists in SSM and bail out (or ask if you want to use the existing project demo and then write the workspace files).

I'm imagining a situation like this:

  1. Efe creates a project called demo with --domain efeiscool.cool
  2. PH wants to create a demo project as well, but with --domain phiscool.cool

With the current code, I think that will work and Efe's project's configuration will get overwritten.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question! Never thought about the case when more then one ppl working on the same project. I guess in the scenario you put, when PH want to create a demo project, if they are using the same AWS account, the CLI should do a check first if it exists in the SSM already. This can also be applied to env init/app init.

@mergify mergify bot merged commit de49678 into aws:master Mar 20, 2020
Sprint 🏃‍♀️ automation moved this from In review to Pending release Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Sprint 🏃‍♀️
  
Pending release
Development

Successfully merging this pull request may close these issues.

env init workflow can't write env SSM entry if user cancels during active deployment
3 participants