refactor(tmp-toolkit-helpers): DiffFormatter class #283
Conversation
kaizencc
had a problem deploying
to
integ-approval
GitHub Actions
Failure
| import type * as cxapi from '@aws-cdk/cx-api'; | ||
| import * as chalk from 'chalk'; | ||
|
|
||
| import { RequireApproval } from '../../api/require-approval'; |
There was a problem hiding this comment.
This should move towards the approach were return a classification and let the CLI deal with interaction.
There was a problem hiding this comment.
yes agree. this will be the first follow up PR as i think its most important
| readonly oldTemplate: any; | ||
| readonly newTemplate: cxapi.CloudFormationStackArtifact, |
There was a problem hiding this comment.
Since these are different types, maybe they do to serve different names.
|
|
||
| export interface FormatSecurityDiffOptions { | ||
| readonly requireApproval: RequireApproval, | ||
| readonly stackName: string, |
There was a problem hiding this comment.
Can we just retrieve this from the stack artifact?
| * | ||
| * @default false | ||
| */ | ||
| readonly quiet?: boolean; |
| /** | ||
| * @default undefined | ||
| */ | ||
| readonly nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates }; |
There was a problem hiding this comment.
Same as above. Can this be retrieved from the stack artifact, as part of this API?
| readonly ioDefaultHelper: IoDefaultMessages; | ||
| } | ||
|
|
||
| export class DiffFormatter { |
There was a problem hiding this comment.
Given how to call site works there might be a larger abstraction hiding. Your DiffFormater could take all options as input, does the shared work between full and security diff, and can return both formats:
const diff = new DiffFormatter(props);
diff.rawDiff // Template diff
diff.fullDiff
diff.securityDiffThis might be preferable for the toolkit.
There was a problem hiding this comment.
agree re: larger abstraction, but for now, i've simply lowered the scope of this PR (or defined it to be what i was initially trying to achieve)
kaizencc
had a problem deploying
to
integ-approval
GitHub Actions
Failure
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
==========================================
- Coverage 85.59% 85.44% -0.15%
==========================================
Files 222 222
Lines 36798 36885 +87
Branches 4468 4462 -6
==========================================
+ Hits 31497 31517 +20
- Misses 5209 5270 +61
- Partials 92 98 +6
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:
|
Signed-off-by: github-actions <[email protected]>
aws-cdk-automation
had a problem deploying
to
integ-approval
GitHub Actions
Failure
kaizencc
had a problem deploying
to
integ-approval
GitHub Actions
Failure
4 participants
the point of this PR is to refactor
formatStackDiffandformatSecurityDiffinto the same class. there's more to do to simplify these functions, but i'm making small refactors a priority.the achievement of this PR is to get rid of the giant laundry list of properties that go into each function in favor of a property bag, and to store the repeated properties within a class.
this is tested by current unit tests -- as long as they still pass, this refactor should have not changed any functionality.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license