feat: add project delete subcommand #609
Conversation
sonofachamp
requested review from
efekarakus,
hencrice,
iamhopaul123 and
kohidave
efekarakus
changed the title
|
Sample run with two environments and one application deployed to both environments: |
| Accounts: aws.StringSlice(accounts), | ||
| RetainStacks: aws.Bool(false), | ||
| Regions: aws.StringSlice(regions), |
There was a problem hiding this comment.
Let's say that we have env_A deployed in account=1,region=us-west-2 and env_B in account=2,region=us-east-1.
Will this call try to delete a stack in account=1,region=us-east-1? In that situation does it fail?
There was a problem hiding this comment.
So here's the scenario I tested:
ecs-preview project init myproj
// profile1 has creds for account A and region us-west-2
ecs-preview env init --name env1 --profile profile1
// profile2 has creds for account B and region us-east-2
ecs-preview env init --name env2 --profile profile2
ecs-preview app init -n myapp -d ./Dockerfile -t "Load Balanced Web App"
ecs-preview app deploy --name myapp --env env1
ecs-preview app deploy --name myapp --env env2
ecs-preview project delete --yes
✔ Deleted app myapp from env env1.
✔ Deleted app myapp from env env2.
✔ Deleted app myapp resources from project myproj.
✔ Deleted app myapp from project myproj.
✔ Deleted project resources.
✔ Deleted environment env1 from project myproj.
✘ Failed to delete environment env2 from project myproj: failed to find a stack named myproj-env2.
✔ Deleted project parameters.
✔ Deleted local workspace folder.
The StackSet instance deletion works fine across multiple accounts and regions. Environment deletion for the first environment worked, but failed for the second environment. At a glance it seems related to deleting environments requiring specific profiles, but need to dig into the details of why that is. It seems strange that one succeeded.
There was a problem hiding this comment.
The stop-gap for now is to use interactivity to enable the user to select a profile for environment deletion:
ecs-preview project delete
? Are you sure you want to delete project myproj? Yes
✔ Deleted app myapp from env env1.
✔ Deleted app myapp from env env2.
✔ Deleted app myapp resources from project myproj.
✔ Deleted app myapp from project myproj.
✔ Deleted project resources.
? Which named profile should we use to delete env1? one
✔ Deleted environment env1 from project myproj.
? Which named profile should we use to delete env2? two
✔ Deleted environment env2 from project myproj.
✔ Deleted project parameters.
✔ Deleted local workspace folder.
| @@ -47,6 +47,28 @@ type deleteAppOpts struct { | |||
| projectEnvironments []*archer.Environment | |||
| } | |||
|
|
|||
| func newDeleteAppOpts() (*deleteAppOpts, error) { | |||
There was a problem hiding this comment.
Feel like this func has duplicated code with how we initialize deleteAppOpts in this file. Maybe we should reuse newDeleteAppOpts() in this file? Otherwise defining a non-exported function in one file and only use it in another file seems to be a little odd to me. What do you think?
There was a problem hiding this comment.
I agree we should reuse the newDeleteAppOpts() function within the BuildAppDeleteCmd() function. I don't want to include that as part of this PR since we'll also need to change the flag bindings to use viper to ensure we can properly handle an error returned from calling this function in the RunE function.
| ) | ||
|
|
||
| var ( | ||
| errOperationCancelled = errors.New("operation cancelled") |
There was a problem hiding this comment.
In "env_delete" when users stop running the log message is "Aborted deletion of environment %s". I think maybe we should return nil and do nothing if they do not confirm instead of erring out? Also keep a consistent user experience when doing "delete".
There was a problem hiding this comment.
After thinking about this a bit I'm torn for a couple of reasons:
- Returning an error feels more explicit and is less likely to be mis-interpreted relative to returning a successful exit code (0) when the expectation is to have deleted something in the happy path case
- Trying to "gracefully handle" it through a non-error introduces more state into the
optsstruct that's then shared between theAskandExecutemethods (both patterns of which I'm not particularly a fan of for the related reasons)
I'm all for a consistent experience, but I don't think it should come at the above costs.
There was a problem hiding this comment.
That makes sense to me. Actually I am ok with either returning an error or successful code 0. I just want our delete experience (project, env, app) to be consistent.
There was a problem hiding this comment.
LGTM! just waiting on https://github.com/aws/amazon-ecs-cli-v2/pull/609/files#r368085704
Addresses #573
Added
project deletesub-command which can be used to wipe out all project related data.Address #573
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.