-
Notifications
You must be signed in to change notification settings - Fork 28
refactor(cli): make the cli use more shared code #207
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
@@ -725,6 +725,8 @@ tmpToolkitHelpers.eslint?.addRules({ | |||
'@typescript-eslint/consistent-type-imports': 'error', | |||
}); | |||
|
|||
tmpToolkitHelpers.gitignore.addPatterns('test/**/*.map'); |
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 idea how this file got there in the first place. But it won't anymore in future.
@@ -184,7 +184,7 @@ export const EVENT_TO_LEVEL: Record<EventType, IoMessageLevel | false> = { | |||
|
|||
export abstract class BasePublishProgressListener implements IPublishProgressListener { | |||
protected readonly ioHost: IIoHost; | |||
protected readonly action: ToolkitAction; | |||
protected readonly action: IoMessaging['action']; |
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.
Plan is to not have the action
here but to have an ActionAwareIoHost
. See importer.ts
as an example. Eventually we will just be passing ActionAwareIoHost
hosts around.
@@ -102,17 +103,15 @@ export class ResourceImporter { | |||
|
|||
private readonly stack: cxapi.CloudFormationStackArtifact; | |||
private readonly cfn: Deployments; | |||
private readonly ioHost: IIoHost; | |||
private readonly action: ToolkitAction; | |||
private readonly ioHost: ActionAwareIoHost; |
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 is the future!
|
||
constructor( | ||
stack: cxapi.CloudFormationStackArtifact, | ||
props: ResourceImporterProps, | ||
) { | ||
this.stack = stack; | ||
this.cfn = props.deployments; | ||
this.ioHost = props.ioHost; | ||
this.action = props.action; | ||
this.ioHost = withAction(props.ioHost, props.action as any); |
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.
Almost the future. Eventually this class should just get an ActionAwareIoHost
in the first place.
@@ -78,7 +78,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n | |||
debug(`Error while checking for platform warnings: ${e}`); | |||
} | |||
|
|||
debug('CDK toolkit version:', version.displayVersion()); | |||
debug('CDK Toolkit CLI version:', version.displayVersion()); |
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.
Just cause.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #207 +/- ##
==========================================
- Coverage 85.27% 85.12% -0.16%
==========================================
Files 207 208 +1
Lines 35837 35775 -62
Branches 4656 4645 -11
==========================================
- Hits 30559 30452 -107
- Misses 5120 5171 +51
+ Partials 158 152 -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:
|
9c39e7f
to
7ef9d82
Compare
4bbfb9d
to
979c0d5
Compare
Added |
Removes duplicated interfaces.
I know this is messy. Please bear with me 🙈
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license