-
Notifications
You must be signed in to change notification settings - Fork 4k
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
chore: unconnected CliIoHost logger-only implementation #32503
Conversation
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32503 +/- ##
==========================================
+ Coverage 80.64% 80.69% +0.05%
==========================================
Files 107 108 +1
Lines 6994 7019 +25
Branches 1290 1299 +9
==========================================
+ Hits 5640 5664 +24
Misses 1175 1175
- Partials 179 180 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
packages/aws-cdk/toolkit/toolkit.ts
Outdated
export const _private = { | ||
CliIoHost, | ||
IoMessageLevel, | ||
IoAction, | ||
} as const; |
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.
What is this?
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.
It's so I can test it without exporting the toolkit just yet. I need to export something so I can write tests
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.
Yeah let's not do that. You can export things from this file. Just not from the package index.
packages/aws-cdk/toolkit/toolkit.ts
Outdated
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.
You probably want to call this file after its contents.
This stuff still needs to be in lib
.
packages/aws-cdk/toolkit/toolkit.ts
Outdated
async notify(msg: IoMessage): Promise<void> { | ||
const output = this.formatMessage(msg); | ||
|
||
const stream = msg.level === 'error' ? process.stderr : process.stdout; |
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.
stream selection doesn't match what we are currently doing in this regard.
packages/aws-cdk/toolkit/toolkit.ts
Outdated
code: string; | ||
message: string; | ||
// Specify Chalk style for stdout/stderr, if TTY is enabled | ||
style?: ((str: string) => string); |
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 a good thought. Is this currently used anywhere?
Beyond that, that - there's an interesting question for you: We currently use chalk
inline in a number of places. How are we going to deal with that?
Might be easier to always chalk and use a global setting to deal with color vs no color.
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.
Not currently used anywhere, but I thought this would be a nice thing to add for customers when we create the IoHost interface
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 did some more recon on this and this is a very real problem I'm gonna need to address in a couple spots. This is enough though that I want to break that out into a separate pr, so I'll handle that in a follow up
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.
can you explain the problem here?
packages/aws-cdk/toolkit/toolkit.ts
Outdated
enum IoMessageLevel { | ||
ERROR = 'error', | ||
WARN = 'warn', | ||
INFO = 'info', | ||
DEBUG = 'debug', | ||
TRACE = 'trace', | ||
} |
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.
Let's make this a type instead of an enum.
packages/aws-cdk/toolkit/toolkit.ts
Outdated
TRACE = 'trace', | ||
} | ||
|
||
enum IoAction { |
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.
Let's make this a type instead of an enum.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue #32345
Closes #32345
Reason for this change
Setting the ground work for our Programmatic Toolkit
Description of changes
Created an unconnected CLIIoHost with a singular initial action available
notify
. In this implementation of the soon to be defined IoHost we are only writing logs to stdout and stderr.Description of how you validated changes
Verified via unit testing as this is currently unconnected to the greater AWS CDK CLI
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license