-
Notifications
You must be signed in to change notification settings - Fork 71
fix(cli): deploy and destroy with --all option fail on apps with no top-level stacks
#985
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
base: main
Are you sure you want to change the base?
Conversation
…top-level stacks fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #985 +/- ##
==========================================
+ Coverage 87.72% 87.98% +0.26%
==========================================
Files 72 72
Lines 10077 10082 +5
Branches 1327 1335 +8
==========================================
+ Hits 8840 8871 +31
+ Misses 1212 1186 -26
Partials 25 25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Head branch was pushed to by a user without write access
807c8bc to
9550003
Compare
…l-on-stage-only
dd751ed to
989cb78
Compare
--all option fails on apps with no top-level stacks--all option fail on apps with no top-level stacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this behavior is desirable or intuitive.
The constraints are:
--allwas introduced before Stages existed. When Stages were introduced, it would continue to select stacks only in the top-level assembly.- Stages are intended to be deployed using a pipeline in the same application. For that use case, you don't really want to select them using
cdk deploy. - I think having a "sometimes
cdk deploydeploys stacks in stages and sometimes not" kind of behavior will be extremely confusing.
I understand this might be desirable for your specific use case, but is your use case represenative?
As written on your issue,
$ cdk deploy '**'
Should solve your particular problem exactly, and it's not that much more to type than --all.
Don't get me wrong: I'm not against making changes here altogether. But I think it should be a co-operation between the CDK app and the CLI. Perhaps a Stack gets a new option that interplays with a new selection mechanism:
new Stack(this, 'SomeStack', {
deployUsingCli: WITH_ALL_FLAG | ONLY_IF_EXPLICITLY_SELECTED | NEVER,
});And then we set appropriate defaults to retain existing behavior.
I'm also not opposed to changing the documentation to better reflect actual behavior.
But as-is, I think changing this behavior in the CLI only will get too confusing.
Fixes: #1003
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license