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

feat(cli): add task delete command #1849

Merged
merged 6 commits into from
Jan 27, 2021
Merged

Conversation

bvtujo
Copy link
Contributor

@bvtujo bvtujo commented Jan 20, 2021

This PR does the following:

  • Adds task delete as a new command
  • Adds a step in app delete during which all existing tasks in the application's environments are deleted
  • Rephrases the e2e test for tasks to use the new command on teardown
  • Adds documentation of the new command.

Depends on #1840 being merged in.

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

@bvtujo bvtujo added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jan 20, 2021
@bvtujo bvtujo requested a review from a team as a code owner January 20, 2021 01:15
@bvtujo bvtujo requested a review from uttarasridhar January 20, 2021 01:15
@bvtujo bvtujo force-pushed the app-delete-tasks branch 2 times, most recently from e3d82d7 to f1032ca Compare January 22, 2021 23:22
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! just some documentation suggestions

3. Delete the CloudFormation stack, including the ECR repo, log group, and ECS Task Definition.

!!!info
1. Tasks created with versions of Copilot earlier than v1.2.0 cannot be stopped by `copilot task delete`. Customers using tasks launched with earlier versions should manually stop any running tasks via the ECS console after running the command.
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the call in task run to ecs.RunTask did not have the PropagateTags: aws.String(ecs.PropagateTagsTaskDefinition) property set so the tasks cannot be reliably identified as Copilot-initiated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The side effect would be the ability to stop tasks from a one-off task definition not started by copilot.

Copy link
Contributor

Choose a reason for hiding this comment

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

ooh yeaa gotcha, thanks for the clarification

@bvtujo bvtujo removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jan 27, 2021
@bvtujo
Copy link
Contributor Author

bvtujo commented Jan 27, 2021

Revised E2E test passes.

...
    /github.com/aws/copilot-cli/e2e/task/task_test.go:128
------------------------------
✔ Stopped all running tasks in family e2e-task-1611705473.
✔ Emptied ECR repositories for task e2e-task-1611705473.
✔ Deleted resources of task e2e-task-1611705473.
Recommended follow-up actions:
✔ Deleted environment test from application e2e-task-1611705473.
✔ Cleaned up deployment resources.
✔ Deleted application resources.
✔ Deleted application configuration.
✔ Deleted local .workspace file.

Ran 7 of 11 Specs in 768.633 seconds
SUCCESS! -- 7 Passed | 0 Failed | 0 Pending | 4 Skipped
PASS

@mergify mergify bot merged commit 3d372a7 into aws:mainline Jan 27, 2021
thrau pushed a commit to localstack/copilot-cli-local that referenced this pull request Dec 9, 2022
<!-- Provide summary of changes -->
This PR does the following:
* Adds `task delete` as a new command
* Adds a step in `app delete` during which all existing tasks in the application's environments are deleted
* Rephrases the e2e test for tasks to use the new command on teardown
* Adds documentation of the new command. 

<!-- Issue number, if available. E.g. "Fixes aws#31", "Addresses aws#42, 77" -->
Depends on aws#1840 being merged in. 

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants