chore: Emit Docker and Shell logging to ProgressListener #221
Conversation
lib/progress.ts
Outdated
| @@ -77,7 +77,7 @@ export interface IPublishProgress { | |||
| /** | |||
| * How far along are we? | |||
| */ | |||
| readonly percentComplete: number; | |||
| readonly percentComplete?: number; | |||
There was a problem hiding this comment.
Percent complete is just a measure of how many assets have been published/built and it's not relevant to what the shell logs are doing so I ignore it when capturing shell logs
There was a problem hiding this comment.
This strongly indicates to me that you are not using the interface called IPublishProgress correctly which defines itself as "Context object for publishing progress".
test/mock-child_process.ts
Outdated
|
|
||
| /** | ||
| * Only match a prefix of the command (don't care about the details of the arguments) | ||
| */ |
There was a problem hiding this comment.
Why are you removing comments?
There was a problem hiding this comment.
I thought it was a bit redundent but I'll add it back
test/mock-child_process.ts
Outdated
| if (invocation.stderr) { | ||
| // Send stderr data after stdout | ||
| if (invocation.stderr) { |
bin/logging.ts
Outdated
| percentComplete: percentComplete, | ||
| abort: () => {}, | ||
| }; | ||
| globalProgressListener.onPublishEvent(logLevelToEventType(level), progressEvent); |
There was a problem hiding this comment.
So this is sending the log message to the globalProgressListener?
Which in turn when using DefaultProgressListener then calls log?
How does that work?
There was a problem hiding this comment.
This is needless right now, but I decided to build it this way as I wanted to provide an easy angle by which someone could add progress listeners even if using the cli. It's a bit pre-emptive so I can remove it if we want as it's un-needed complexity at this point.
There was a problem hiding this comment.
If you are removing progress listeners, what is left in this PR?
My point here was the loop you are creating:
log() -> globalProgressListener.onPublishEvent() -> DefaultProgressListener -> log()
There was a problem hiding this comment.
It's also not tested. Pretty sure if you had a test with DefaultProgressListener you would notice that.
There was a problem hiding this comment.
but I decided to build it this way as I wanted to provide an easy angle by which someone could add progress listeners even if using the cli.
That's an okay decision. You need to explain these decisions in your PR descrption.
bin/logging.ts
Outdated
| if (isError) { | ||
| process.stderr.write(text); | ||
| } else { | ||
| process.stdout.write(text); | ||
| } |
There was a problem hiding this comment.
So we are still writing directly to stderr and stdout?
There was a problem hiding this comment.
Yeah we are doing both for now. I could detect if we are being called directly from publish and ignore printing to stdout but I figured it wouldn't hurt
There was a problem hiding this comment.
Anything written to process.stdout or process.stderr needs also be pluggable
That's the whole point of the work: That nothing is indiscriminately written to stdout/stderr anymore.
BTW, if it doesn't close the issue, e.g. because a second PR is needed, you cannot put that in like that. |
HBobertz
changed the title
HBobertz
changed the title
Fixes #196
There is still a quiet flag however I will do that in another pr as I need to get rid of it in the CDK as well and will probably do it there first