fix(aws-cdk): cdk diff always fails on diff
#17862
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
njlynch
changed the title
cdk diff always fails on diffcdk diff always fails on diff
In v1, the original behavior was that `cdk diff` exited 0 on a clean diff (no changes), and exited 1 on changes. #4721 introduced a new feature flag to always have `cdk diff` exit 0. As is standard, the "default" for the flag was `false`, but the future behavior was `true`, meaning any new CDK project created after that release (v1.19.0) defaulted to always exiting 0. In v2, the feature flag was expired, meaning users could no longer set it. Typically, the behavior (`FeatureFlags.isEnabled`) checks and errors if an expired flag is explicitly set, and always returns true for expired flags otherwise. However, the CDK CLI helper function did not contain this functionality. That means for current v2 projects, the v1-ported default value is used (false). This means `cdk diff` *always* exits 1 when there's a diff, and there's no way for a user to disable this behavior. This change updates the CLI behavior to default to true for expired flags, same as any other feature flag check; this means `cdk diff` will return to exiting 0, as v1 apps have been doing for years. The `FeatureFlags.isEnabled` method can't be used, as it requires a Construct node. I also took the liberty of explicitly setting the "default" to true for all expired flags, so any other direct access of the flag (not using the FeatureFlag library) will have the same consistent behavior.
njlynch
force-pushed
the
njlynch/v2-feature-flag-fixup
branch
from
4290607 to
840d8f3
Compare
njlynch
added
pr-linter/exempt-test
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
rix0rrr
approved these changes
Dec 6, 2021
rix0rrr
approved these changes
Dec 6, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In v1, the original behavior was that
cdk diffexited 0 on a clean diff (nochanges), and exited 1 on changes. #4721 introduced a new feature flag to always
have
cdk diffexit 0. As is standard, the "default" for the flag wasfalse,but the future behavior was
true, meaning any new CDK project created afterthat release (v1.19.0) defaulted to always exiting 0.
In v2, the feature flag was expired, meaning users could no longer set
it. Typically, the behavior (
FeatureFlags.isEnabled) checks and errors if anexpired flag is explicitly set, and always returns true for expired flags
otherwise. However, the CDK CLI helper function did not contain this
functionality. That means for current v2 projects, the v1-ported default value
is used (false). This means
cdk diffalways exits 1 when there's a diff, andthere's no way for a user to disable this behavior.
This change updates the CLI behavior to default to true for expired flags, same
as any other feature flag check; this means
cdk diffwill return to exiting 0,as v1 apps have been doing for years. The
FeatureFlags.isEnabledmethod can'tbe used, as it requires a Construct node.
I also took the liberty of explicitly setting the "default" to true for all
expired flags, so any other direct access of the flag (not using the FeatureFlag
library) will have the same consistent behavior.
fixes #17832
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license