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

chore(cli): add task delete validate/ask #1831

Merged
merged 12 commits into from
Jan 16, 2021
Merged

Conversation

bvtujo
Copy link
Contributor

@bvtujo bvtujo commented Jan 14, 2021

Adds the skeleton of a new task delete command with Ask() and Validate() filled in. Tests for Ask() forthcoming, then I'll work on Execute in a separate PR.

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

bvtujo added 7 commits January 8, 2021 20:55
chore(aws): add ListStacksWithTags method

chore(aws): remove unused client call from interfaces

chore(cloudformation): add ListStacksWIthTags to cf interfaces

chore(cloudformation): extract task stack name pattern to variable in deploy pkg

chore(awscfn): fix staticcheck error

chore(cloudformation): address feedback

chore(cloudformation): revert invalid change to ListDefaultTaskStacks

chore(cloudformation): regenerate mocks

chore(cloudformation): remove unnecessary private method
@bvtujo bvtujo requested a review from a team as a code owner January 14, 2021 01:11
@bvtujo bvtujo requested a review from uttarasridhar January 14, 2021 01:11
// Validate checks that flag inputs are valid.
func (o *deleteTaskOpts) Validate() error {

if o.name != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably missing it-- do we validate that the flag-inputted task exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It's difficult because in the case where a task is provided but no app, env, or default cluster flag, we don't know where to look for the task.

In these cases we rely on error messages returned when we go looking for the task or service in the Execute logic.

Copy link
Contributor

@huanjani huanjani left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

LGTM just a few nits.

internal/pkg/cli/interfaces.go Outdated Show resolved Hide resolved
internal/pkg/cli/task_delete.go Outdated Show resolved Hide resolved
internal/pkg/cli/task_delete.go Show resolved Hide resolved
internal/pkg/cli/task_delete.go Outdated Show resolved Hide resolved
Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

LGTM! just one nit. feel free to put down the label.

deleteTaskVars: vars,

store: store,
// spinner: termprogress.NewSpinner(log.DiagnosticWriter),
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh maybe this one as well.

@iamhopaul123 iamhopaul123 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jan 16, 2021
@bvtujo bvtujo removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jan 16, 2021
@mergify mergify bot merged commit 79c1654 into aws:mainline Jan 16, 2021
thrau pushed a commit to localstack/copilot-cli-local that referenced this pull request Dec 9, 2022
<!-- Provide summary of changes -->
Adds the skeleton of a new `task delete` command with Ask() and Validate() filled in. Tests for Ask() forthcoming, then I'll work on Execute in a separate PR. 

<!-- Issue number, if available. E.g. "Fixes aws#31", "Addresses aws#42, 77" -->

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