-
Notifications
You must be signed in to change notification settings - Fork 35
feat(toolkit-lib): diff #279
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
+ Coverage 85.34% 85.43% +0.09%
==========================================
Files 222 222
Lines 36885 36898 +13
Branches 4448 4451 +3
==========================================
+ Hits 31478 31524 +46
+ Misses 5310 5280 -30
+ Partials 97 94 -3
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:
|
@@ -12,6 +12,7 @@ export { CloudWatchLogEventMonitor, findCloudWatchLogGroups } from '../../../../ | |||
export { type WorkGraph, WorkGraphBuilder, AssetBuildNode, AssetPublishNode, StackNode, Concurrency } from '../../../../aws-cdk/lib/api/work-graph'; | |||
export { Bootstrapper } from '../../../../aws-cdk/lib/api/bootstrap'; | |||
export { loadTree, some } from '../../../../aws-cdk/lib/api/tree'; | |||
export { createDiffChangeSet } from '../../../../aws-cdk/lib/api/deployments/cfn-api'; |
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 also need to use this function and there's not a good way of moving it to tmp-toolkit-helpers because i would basically be moving the entire deployments
folder. wonder if this is ok for now
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.
Yes fine. Will move soon. But add it to the existing import further up, you might need to export the function from the api module.
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.
ok, but doing so means that i export the cfn-api
file out of aws-cdk
expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ | ||
action: 'diff', | ||
level: 'warn', | ||
code: 'CDK_TOOLKIT_W0000', | ||
message: expect.stringContaining('This deployment will make potentially sensitive changes according to your current security approval level (--require-approval broadening)'), | ||
})); |
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.
sanity check: do we expect to have this as a warning notification when we run toolkit.diff
?
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.
No, probably not.
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.
this comes from here
it was previously a warning()
before being refactored into ioDefaultHelper.warning
, so it was always printed as a warning even when the command was cdk diff
(and not cdk deploy
, where presumably you would need to stop and confirm the diff). cdk deploy
has its own confirm message that actually prompts the user.
i think we should be able to just delete those two lines -- i don't think they are useful. alternatively, i don't think its harmful to leave them in.
Signed-off-by: github-actions <[email protected]>
introduces diff support for programmatic toolkit.
does most of aws/aws-cdk#33182, but typed return will come later
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license