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 --cluster flag to task run #2164

Merged
merged 8 commits into from
Apr 15, 2021

Conversation

Lou1415926
Copy link
Contributor

@Lou1415926 Lou1415926 commented Apr 9, 2021

Allow user to specify the cluster to run the task in.

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

@Lou1415926 Lou1415926 requested a review from a team as a code owner April 9, 2021 23:36
@Lou1415926 Lou1415926 requested a review from iamhopaul123 April 9, 2021 23:36
@Lou1415926 Lou1415926 changed the title chore(cli): add --clsuter flag to task run chore(cli): add --cluster flag to task run Apr 9, 2021
@efekarakus efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 13, 2021
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! Great tests!

@efekarakus efekarakus removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 14, 2021
@Lou1415926 Lou1415926 changed the title chore(cli): add --cluster flag to task run feat(cli): add --cluster flag to task run Apr 14, 2021
@@ -132,6 +133,8 @@ Mutually exclusive with -%s, --%s`, imageFlagShort, imageFlag)
wkldTypeFlagDescription = fmt.Sprintf(`Type of job or svc to create. Must be one of:
%s`, strings.Join(template.QuoteSliceFunc(manifest.WorkloadTypes), ", "))

clusterFlagDescription = fmt.Sprintf(`Optional. The ARN of the cluster to run the task in.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update this to say that they could also pass the short name for the cluster so that it works similar to run-task?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there is an opportunity for us to simplify the --default and --cluster behavior 🤔

@huanjani discovered that we can hide flags with cobra:

_ = cmd.Flags().MarkHidden(githubURLFlag)

I wonder if we should now hide --default and if --cluster is specified with no --subnets we can default to the using default VPC subnets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(After an extensive search into my memory) I recalled that the --default flag was introduced in order to skip prompting for app and env. It is completely an UX thing and does nothing to the ConfigRunner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this fail to run if the cluster doesn't exist in the default vpc? I want to make sure we're defaulting to the correct thing. Can we check under the hood whether the cluster has copilot tags, then use the correct subnets automagically?

@efekarakus
Copy link
Contributor

Code looks great btw! just the suggestions above

@Lou1415926 Lou1415926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 14, 2021
@Lou1415926 Lou1415926 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 14, 2021
@mergify mergify bot merged commit 32472a9 into aws:mainline Apr 15, 2021
Lou1415926 added a commit to Lou1415926/copilot-cli that referenced this pull request Apr 20, 2021
Allow user to specify the cluster to run the task in.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Lou1415926 added a commit to Lou1415926/copilot-cli that referenced this pull request Apr 20, 2021
Allow user to specify the cluster to run the task in.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
thrau pushed a commit to localstack/copilot-cli-local that referenced this pull request Dec 9, 2022
Allow user to specify the cluster to run the task 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.

4 participants