0.13 cherrypick logger changes #3601
Conversation
This commit makes it harder to forget sending verbose logs (mostly tool output) to both Garden Cloud and to the CLI logs. To accomplish that, this commit 1. Fixed the type assertions in the PluginEventBroker. Now typescript will actually verify the types for the events in `emit` and in `on` etc. 2. Added two additional parameters to the `log` event, origin and log. Log is supposed to be a LogEntry placeholder with verbose log level. 3. Make sure that we use the `ctx.events.emit` in the plugin implementations to stream logs to Garden Cloud and CLI
This essentially reverts a change that was made in #3576 (commit: b52e9b2). The type has always been 'number' but we were in fact sending strings to Garden Cloud which is the payload it expects. Garden Cloud (version >= 1.360) now supports numbers but we need to continue sending strings for backwards compatibility. We can tighten the type to just 'number' once all Cloud instances are up to date.
This event was introduced awhile ago but isn't used by any client at the moment. It was made into its own event type by mistake, it should've just been a normal event with the name 'serverReady'. This fixes that.
Actually stream helm module outputs to CLI and Garden Cloud; the log context was in the wrong place before.
| params.events.on("log", ({ timestamp, data }) => { | ||
| params.events.on("log", ({ timestamp, data, origin, log }) => { | ||
| // stream logs to CLI | ||
| log.setState(renderOutputStream(data.toString(), origin)) |
There was a problem hiding this comment.
For some reason, in 0.13, the "log" handlers are not being called. I tried to understand why, but couldn't find the culprit. My guess is that the events instance in the plugin is a different instance than here.
This means that once this PR is merged, some logs won't appear in the CLI anymore until this "unrelated" problem has been fixed.
There was a problem hiding this comment.
It is as you suspect, a different instance (params.events vs. ctx.events). We may want to drop one of them actually to avoid that confusion going forward. I can take that on.
There was a problem hiding this comment.
I've added a commit that ensures the same event broker is passed along through the plugin router, which should fix this issue.
edvald
force-pushed
the
0.13-cherrypick-logger-changes
branch
from
fd337e4 to
e305f16
Compare
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: