cdklabs / cdk-assets

Apache License 2.0
3 stars 2 forks source link

Logging needs to be pluggable #196

Open mrgrain opened 5 days ago

mrgrain commented 5 days ago

Currently cdk-assets writes logs directly to stdout and stderr. It also uses a boolean quiet flag to decide if output is printed or not.

rix0rrr commented 14 hours ago

Are you sure we need this? The cdk-assets library already exposes a pluggable logger interface:

    /**
     * Listener for progress events
     *
     * @default No listener
     */
    readonly progressListener?: IPublishProgressListener;

That is used by the CDK CLI. CDK CLI funnels log events to the regular CDK CLI logger.

mrgrain commented 13 hours ago

Are you sure we need this? The cdk-assets library already exposes a pluggable logger interface: That is used by the CDK CLI. CDK CLI funnels log events to the regular CDK CLI logger.

Great find! In that case we just need to make sure this is used everywhere. I came across the Docker asset building implementation which is not using the logger at the moment:

https://github.com/cdklabs/cdk-assets/blob/79c76a7f766ef3d768b21706114d5b66b3169bbc/lib/private/shell.ts#L38

rix0rrr commented 13 hours ago

I was discussing this with @HBobertz today, and we're not quite sure what the best solution is here. This is for build output that is intended primarily for a human's eyeballs, not for a program. Of course, in an IDE context or whatever, we would want to capture it so we can send it somewhere else. Buffering stdout to the last moment and then emitting it all at once is probably a bad experience, so we'll need to do something on-the-fly. Also, the chunks can unpredictably range anywhere from 1 to 8092 bytes.

What do you think of the following approach: