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): --all flag to select all stacks #10745

Conversation

IsmaelMartinez
Copy link
Contributor

@IsmaelMartinez IsmaelMartinez commented Oct 6, 2020

Adding the --all option to deploy all stacks.

cdk deploy --all

This is an alias of *. The previous command does the same as the following one:

cdk deploy '*'

Closes #3222


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@IsmaelMartinez IsmaelMartinez changed the title feature(core): Add cdk deploy --all to the client options feature(core): add cdk deploy --all to the client options Oct 6, 2020
@IsmaelMartinez IsmaelMartinez changed the title feature(core): add cdk deploy --all to the client options feature(core): add cdk deploy --all to the client options Oct 7, 2020
@IsmaelMartinez
Copy link
Contributor Author

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

Can someone re-trigger this? I have changed twice the title but can't see this triggering. PR should be ready now for review. Thanks

@IsmaelMartinez IsmaelMartinez mentioned this pull request Oct 7, 2020
5 tasks
@Chriscbr
Copy link
Contributor

Chriscbr commented Oct 7, 2020

I'm wondering if it might be better to have the command error instead if both "--all" and a list of stacks are provided. I feel like this could help catch user errors, considering that this implementation looks slightly different from how some other CLI tools operate (e.g. in Git, if you run git add --all foo then only foo will be added to your staged files, ignoring the flag).

Can someone re-trigger this? I have changed twice the title but can't see this triggering. PR should be ready now for review.

I think the title needs to be update to say "feat" instead of "feature"

@IsmaelMartinez IsmaelMartinez changed the title feature(core): add cdk deploy --all to the client options feat(core): add cdk deploy --all to the client options Oct 7, 2020
@IsmaelMartinez
Copy link
Contributor Author

I'm wondering if it might be better to have the command error instead if both "--all" and a list of stacks are provided. I feel like this could help catch user errors, considering that this implementation looks slightly different from how some other CLI tools operate (e.g. in Git, if you run git add --all foo then only foo will be added to your staged files, ignoring the flag).

Can someone re-trigger this? I have changed twice the title but can't see this triggering. PR should be ready now for review.

I think the title needs to be update to say "feat" instead of "feature"

Thanks. I will see when I got another bit of time to add the error message. Sounds reasonable.

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 8, 2020

I'm not a big fan of this. We don't need this flag as cdk deploy \* already exists (as you yourself noted).

Where's the added benefit?

@IsmaelMartinez
Copy link
Contributor Author

IsmaelMartinez commented Oct 8, 2020

I'm not a big fan of this. We don't need this flag as cdk deploy \* already exists (as you yourself noted).

Where's the added benefit?

Only created the PR because there is an open ticket.

Happy to change the PR content to add the unit test for the cdk deploy '*' (was not existent before) and the documentation (if that is needed), but better also close the ticket #3222 or another person might end up doing the same PR.

@Chriscbr
Copy link
Contributor

Chriscbr commented Oct 8, 2020

Only my 2 cents, but the cdk design guidelines suggests:

It's okay to enable multiple ways to achieve the same thing, in order to make it more natural for users who come from different mental models.

What kinds of downsides come to mind?

@IsmaelMartinez
Copy link
Contributor Author

Hi @rix0rrr and @Chriscbr,

In my opinion, there are 3 options on this (on order of preference):

  1. default to deploying all stacks if no list of stacks is provided.
  2. don't add the --all but document it and add unit test to it.
  3. add the --all and add an exception if the user also provides the flag.

In my opinion, it is a bit strange that you need to provide an escaped * as an extra argument (or the list of stack names) if you got more than one stack in an app.

Saying that, it will be a change on functionality but it sort of makes more sense than the current behaviour. It is not that you need to provide your stack name if you only got one.

Let me know if you prefer to put this sort fo conversations in the ticket instead of in the PR.

Ta

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

After having thought on it, I'm willing to accept this.

Please add an input to the CLI that if --all is provided, stack names are not allowed (and add a descriptive error message).

… stacks at the same time

feat: adding the --all option to the destroy command
@mergify mergify bot dismissed rix0rrr’s stale review October 24, 2020 11:21

Pull request has been modified.

@IsmaelMartinez
Copy link
Contributor Author

Hi @rix0rrr and @Chriscbr,

I pushed a few changes. Touched the "destroy" command also as it has the same flow/logic as the deploy and will otherwise work inconsistently.

I checked the synth, list and diff and they don't need the --all as that is their default behaviour (if you don't specify a list of stacks, they just assume all.

Also, do you want me to edit the https://docs.aws.amazon.com/cdk/latest/guide/cli.html github to ensure this is in sync? Not sure if that automatically updates or you have an internal process to do this.

Thanks in advance!

@gitpod-io
Copy link

gitpod-io bot commented Oct 24, 2020

@rix0rrr rix0rrr changed the title feat(core): add cdk deploy --all to the client options feat(cli): --all flag to select all stacks Oct 26, 2020
@mergify
Copy link
Contributor

mergify bot commented Oct 26, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ac827a7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Oct 26, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit bcd9d0a into aws:master Oct 26, 2020
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.

feature: cdk deploy --all
4 participants