feat: add "pipeline delete" command #652

SoManyHs · 2020-02-12T09:48:09Z

Closes #630

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

efekarakus

Looks really good, thank you!

internal/pkg/cli/pipeline_delete.go

internal/pkg/store/secretsmanager/secretsmanager.go

internal/pkg/cli/pipeline_delete.go

efekarakus · 2020-02-12T18:09:46Z

internal/pkg/cli/pipeline_delete.go

+	if err := o.deleteSecret(); err != nil {
+		return err
+	}
+
+	if err := o.deleteStack(); err != nil {
+		return err
+	}


Can we ignore the errors if the secret and stack are already deleted?

Maybe we can write something like log.Successf("Secret %s is already removed.", o.PipelineSecret)

Oh, is this an error we handle like this elsewhere? I was mostly following suit of what app_delete.go does for the stack deletion. On the underlying cloudformation.DeleteStack method we already wrap some errors. Wondering if you think we should add some sugar on top of that?

For the deletion of secret, the errors returned by secretsmanager.DeleteSecret don't seem to return like, an idempotency error. Unless I'm misunderstanding what you mean when you say these things are already deleted?

app_delete calls delete-stack API which does not return an error if stack does not exist. However describe-stacks will return ValidationError if stack does not exist. Maybe we can use delete-stack in pipeline delete as well so that we don't need to validate if pipeline already exists?

Okay so, with the latest changes (see: 3b9bf4c) here's what's happening with the following workflow:

Runpipeline init - creates new secret and writes pipeline.yml

Runpipeline update - deploys pipeline via CFN

From the AWS Console, I schedule the deletion of the secret I just made through pipeline init. This schedules the secret for deletion in N days.

run pipeline delete :

~/aws-reinvent-2019-trivia-backend(master)$ ecs-preview pipeline delete ? Are you sure you want to delete pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend from project backend? Yes ? Are you sure you want to delete the source secret github-token-backend-aws-reinvent-trivia-2019-backend associated with pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend? Yes ✔ Deleted secret github-token-backend-aws-reinvent-trivia-2019-backend. ✔ Deleted pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend from project backend. ~/aws-reinvent-2019-trivia-backend(master)$ aws secretsmanager list-secrets --region us-east-2 { "SecretList": [ ] }

So as far as idempotency for deleting the secret, it seems to be okay for both deleting the secret and the stack (since the stack only refers to the name of the secret, and does not actually create it so there wouldn't be any stack drift).

app_delete calls delete-stack API which does not return an error if stack does not exist. However describe-stacks will return ValidationError if stack does not exist. Maybe we can use delete-stack in pipeline delete as well so that we don't need to validate if pipeline already exists?

I'm not seeing this in app_delete or in env_delete -- am I missing something? @iamhopaul123

Here:

https://github.com/aws/amazon-ecs-cli-v2/blob/5db6427337f08f7ac9e5ba0b2f38f324a085f53e/internal/pkg/cli/app_delete.go#L192

https://github.com/aws/amazon-ecs-cli-v2/blob/1a91c0ea9a5025e6b9ff85a109cee06efb8350b4/internal/pkg/deploy/cloudformation/stack.go#L14-L19

internal/pkg/store/secretsmanager/secretsmanager.go

efekarakus · 2020-02-12T18:37:12Z

oh can you update the git commit to follow: https://www.conventionalcommits.org/en/v1.0.0/

feat: add "pipeline delete" command

iamhopaul123

LGTM! Except for the idempotency issue since we possibly need to delete both secret and CFN stack.

internal/pkg/store/secretsmanager/secretsmanager.go

iamhopaul123 · 2020-02-12T19:09:52Z

internal/pkg/deploy/cloudformation/pipeline.go

+
+func (cf CloudFormation) DeletePipeline(stackName string) error {
+	// Check if the stack exists
+	out, err := cf.describeStack(&cloudformation.DescribeStacksInput{


Same idempotency caveat here. If CFN stack has been deleted.

I think what @iamhopaul123 means is that we can actually remove the describeStack call and call directly cf.delete(stackName) which handles ignoring the stackDoesNotExist error

SoManyHs · 2020-02-12T19:47:15Z

Oh. Would we want to delete the pipeline manifest if we delete the pipeline? It doesn't look like we delete the app manifest when we call app delete, but wondering if there's a reason to keep them around? @efekarakus

efekarakus · 2020-02-12T20:10:43Z

Oh. Would we want to delete the pipeline manifest if we delete the pipeline? It doesn't look like we delete the app manifest when we call app delete, but wondering if there's a reason to keep them around? @efekarakus

We delete them in app delete:
https://github.com/aws/amazon-ecs-cli-v2/blob/b223142e566e3bd123b541ff8cc500d4b3a113fb/internal/pkg/cli/app_delete.go#L264-L271

SoManyHs · 2020-02-14T01:37:55Z

Noting here for my own bookkeeping:
Ran pipeline init and pipeline update, then deleted the CFN stack for the pipeline via the AWS console. Running pipeline delete results in:

~/aws-reinvent-2019-trivia-backend(master)$ ecs-preview pipeline delete


? Are you sure you want to delete pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend from project backend? Yes


? Are you sure you want to delete the source secret github-token-backend-aws-reinvent-trivia-2019-backend associated with pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend? Yes
✔ Deleted secret github-token-backend-aws-reinvent-trivia-2019-backend.
✘ Failed to delete pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend from project backend: failed to find a stack named backend-pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend.
Error: failed to find a stack named backend-pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend

We should try to handle this gracefully.

Also, might want to test this if the pipeline is deleted from the CodePipeline (rather than the CFN) console manually.

efekarakus · 2020-02-14T18:28:25Z

internal/pkg/aws/secretsmanager/secretsmanager.go

+	return []*secretsmanager.Tag{
+		{
+			Key:   aws.String("ecs-project"),
+			Value: aws.String(timestamp),


Should this be the project name instead?

efekarakus · 2020-02-14T18:31:26Z

internal/pkg/deploy/cloudformation/pipeline.go

+
+func (cf CloudFormation) DeletePipeline(stackName string) error {
+	// Check if the stack exists
+	out, err := cf.describeStack(&cloudformation.DescribeStacksInput{


I think what @iamhopaul123 means is that we can actually remove the describeStack call and call directly cf.delete(stackName) which handles ignoring the stackDoesNotExist error

Closes aws#630

This was leftover from a refactor in which we moved much of the PreRunE logic into the constructor for the opts. This approach allows for ease and consistency of testing. NOTE: See 16d3bb1 -- previously, calling the constructors outside of RunE made it harder to catch errors returned by the constructor itself. This allows the errors to be caught will still getting values from the command's flags..

Neither field will be passed in as flags, so they can be moved out of deletePipelineVars.

Since we always assume the pipeline for a workspace will be named "pipeline.yml", we are not passing this field in as a flag.

pending changes in aws#657

iamhopaul123

LGTM! As long as the deletion can continue and return 0 even if the pipeline stack has already been removed manually before; and secret manager won't yell at me if the secret's not exist before pipeline delete.

SoManyHs · 2020-02-17T00:58:54Z

~/aws-reinvent-2019-trivia-backend(master)$ ecs-preview pipeline init

? Would you like to add an environment to your pipeline? Yes

? Which environment would you like to add to your pipeline? test

? Which GitHub repository would you like to use for your application? [email protected]:SoManyHs/aws-reinvent-trivia-2019-backend

? Please enter your GitHub Personal Access Token for your repository: aws-reinvent-trivia-2019-backend [? for help] *****
✔ Secret already exists for aws-reinvent-trivia-2019-backend! Do nothing.
✔ Wrote the pipeline manifest for aws-reinvent-trivia-2019-backend at 'ecs-project/pipeline.yml'
✔ Wrote the buildspec for the pipeline's build stage at 'ecs-project/buildspec.yml'
The manifest contains configurations for your CodePipeline resources, such as your pipeline stages and build steps.
The buildspec contains the commands to build and push your container images to your ECR repositories.

Recommended follow-up actions:
- Update the build phase of your buildspec to unit test your applications before pushing the images.
- Update your pipeline manifest to add additional stages.
- Run `ecs-preview pipeline update` to deploy your pipeline for the repository.

~/aws-reinvent-2019-trivia-backend(master)$ ecs-preview pipeline update
✔ Successfully added pipeline resources to your project: backend
✔ Successfully created a new pipeline: pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend

~/aws-reinvent-2019-trivia-backend(master)$ aws secretsmanager list-secrets
{
    "SecretList": [
        {
            "Name": "github-token-backend-aws-reinvent-trivia-2019-backend", 
            "Tags": [
                {
                    "Value": "Mon Feb 17 00:54:21 UTC 2020", 
                    "Key": "ecs-project"
                }
            ], 
            "LastChangedDate": 1581901027.553, 
            "SecretVersionsToStages": {
                "872299C7-E403-4C5C-83C2-C69CF769BE36": [
                    "AWSCURRENT"
                ]
            }, 
            "LastAccessedDate": 1581897600.0, 
            "ARN": "arn:aws:secretsmanager:us-east-2:xxxxxxxx:secret:github-token-backend-aws-reinvent-trivia-2019-backend-tcA2LL"
        }
    ]
}

~/aws-reinvent-2019-trivia-backend(master)$ ls ecs-project/
api-app.yml	backend-app.yml	buildspec.yml	pipeline.yml

~/aws-reinvent-2019-trivia-backend(master)$ ecs-preview pipeline delete


? Are you sure you want to delete pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend from project backend? Yes


? Are you sure you want to delete the source secret github-token-backend-aws-reinvent-trivia-2019-backend associated with pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend? Yes
✔ Deleted secret github-token-backend-aws-reinvent-trivia-2019-backend.
✔ Deleted pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend from project backend.
✔ Deleted pipeline manifest from workspace.

SoManyHs · 2020-02-17T01:08:16Z

When running pipeline delete after pipeline stack has been deleted manually:

~/aws-reinvent-2019-trivia-backend(master)$ ecs-preview pipeline init

? Would you like to add an environment to your pipeline? Yes

? Which environment would you like to add to your pipeline? test

? Which GitHub repository would you like to use for your application? [email protected]:SoManyHs/aws-reinvent-trivia-2019-backend

? Please enter your GitHub Personal Access Token for your repository: aws-reinvent-trivia-2019-backend [? for help] ******
✔ Created the secret github-token-backend-aws-reinvent-trivia-2019-backend for pipeline source stage!
✔ Wrote the pipeline manifest for aws-reinvent-trivia-2019-backend at 'ecs-project/pipeline.yml'
✔ Wrote the buildspec for the pipeline's build stage at 'ecs-project/buildspec.yml'
The manifest contains configurations for your CodePipeline resources, such as your pipeline stages and build steps.
The buildspec contains the commands to build and push your container images to your ECR repositories.

Recommended follow-up actions:
- Update the build phase of your buildspec to unit test your applications before pushing the images.
- Update your pipeline manifest to add additional stages.
- Run `ecs-preview pipeline update` to deploy your pipeline for the repository.

~/aws-reinvent-2019-trivia-backend(master)$ ecs-preview pipeline update
✔ Successfully added pipeline resources to your project: backend
✔ Successfully created a new pipeline: pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend

~/aws-reinvent-2019-trivia-backend(master)$ ecs-preview pipeline delete


? Are you sure you want to delete pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend from project backend? Yes


? Are you sure you want to delete the source secret github-token-backend-aws-reinvent-trivia-2019-backend associated with pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend? Yes
✔ Deleted secret github-token-backend-aws-reinvent-trivia-2019-backend.
✔ Deleted pipeline pipeline-backend-SoManyHs-aws-reinvent-trivia-2019-backend from project backend.
✔ Deleted pipeline manifest from workspace.

The message isn't quite right, but can fix in a future PR.

SoManyHs requested a review from a team as a code owner February 12, 2020 09:48

SoManyHs requested review from clareliguori and removed request for clareliguori February 12, 2020 09:48

efekarakus reviewed Feb 12, 2020

View reviewed changes

clareliguori changed the title ~~Delete pipeline~~ feat: add "pipeline delete" command Feb 12, 2020

iamhopaul123 reviewed Feb 12, 2020

View reviewed changes

SoManyHs added the area/pipeline Issues about pipelines to release applications. label Feb 13, 2020

SoManyHs force-pushed the pipeline-delete branch 2 times, most recently from 3194e26 to 705e5ca Compare February 14, 2020 01:29

efekarakus approved these changes Feb 14, 2020

View reviewed changes

SoManyHs added 8 commits February 14, 2020 14:35

Delete pipeline

4a96319

Closes aws#630

Move PipelineName and PipelineSecret into opts

30d9358

Neither field will be passed in as flags, so they can be moved out of deletePipelineVars.

Wrap error from secretsmanager.DeleteSecret

a574824

remove PipelineFilename from initPipelineVars

79cda2c

Since we always assume the pipeline for a workspace will be named "pipeline.yml", we are not passing this field in as a flag.

delete pipeline file - placeholder

f780bf2

pending changes in aws#657

Force delete the pipeline secret

291cf88

Add tag for secret

8a448eb

SoManyHs force-pushed the pipeline-delete branch from 705e5ca to c00e021 Compare February 14, 2020 22:38

iamhopaul123 approved these changes Feb 14, 2020

View reviewed changes

SoManyHs force-pushed the pipeline-delete branch 2 times, most recently from 5c01605 to 34dc68b Compare February 17, 2020 00:57

Delete pipeline manifest

def193d

SoManyHs force-pushed the pipeline-delete branch from 34dc68b to def193d Compare February 17, 2020 01:10

SoManyHs mentioned this pull request Feb 17, 2020

Better log message for pipeline delete #668

Closed

SoManyHs merged commit 4612ad5 into aws:master Feb 17, 2020

SoManyHs deleted the pipeline-delete branch February 17, 2020 01:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add "pipeline delete" command #652

feat: add "pipeline delete" command #652

SoManyHs commented Feb 12, 2020

efekarakus left a comment

efekarakus Feb 12, 2020

SoManyHs Feb 12, 2020

iamhopaul123 Feb 12, 2020

SoManyHs Feb 14, 2020

SoManyHs Feb 14, 2020 •

edited

Loading

iamhopaul123 Feb 14, 2020

efekarakus commented Feb 12, 2020

iamhopaul123 left a comment

iamhopaul123 Feb 12, 2020

efekarakus Feb 14, 2020

SoManyHs commented Feb 12, 2020

efekarakus commented Feb 12, 2020

SoManyHs commented Feb 14, 2020 •

edited

Loading

efekarakus Feb 14, 2020

efekarakus Feb 14, 2020

iamhopaul123 left a comment •

edited

Loading

SoManyHs commented Feb 17, 2020

SoManyHs commented Feb 17, 2020

feat: add "pipeline delete" command #652

feat: add "pipeline delete" command #652

Conversation

SoManyHs commented Feb 12, 2020

efekarakus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoManyHs Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efekarakus commented Feb 12, 2020

iamhopaul123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoManyHs commented Feb 12, 2020

efekarakus commented Feb 12, 2020

SoManyHs commented Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamhopaul123 left a comment • edited Loading

Choose a reason for hiding this comment

SoManyHs commented Feb 17, 2020

SoManyHs commented Feb 17, 2020

SoManyHs Feb 14, 2020 •

edited

Loading

SoManyHs commented Feb 14, 2020 •

edited

Loading

iamhopaul123 left a comment •

edited

Loading