improvement(core): better action lifecycle logs #5428
Conversation
| if (!msg) { | ||
| return "" | ||
| } | ||
| // For log levels higher than "info" we print the log level name. |
There was a problem hiding this comment.
I moved the log level name to the log line proper.
So now it's:
section => [level name] message
as opposed to:
section [level name] => message
eysi09
force-pushed
the
action-log-improvements
branch
from
9ded64c
to
3c05841
Compare
eysi09
force-pushed
the
action-log-improvements
branch
8 times, most recently
from
5533a26
to
c8b42ec
Compare
| /** | ||
| * A map of all the colors we use to render text in the terminal. | ||
| */ | ||
| const theme = { | ||
| primary: chalk.grey as unknown as Styles, |
There was a problem hiding this comment.
This was just plain wrong so I removed it.
| `${styles.primary("[debug]")} ${styles.primary("hello world")}\n` | ||
| ) | ||
| }) | ||
| it("should print the log level if it's higher then 'info' after the section if there is one", () => { |
There was a problem hiding this comment.
I changed this behaviour so we can remove this test. There's already a test case for the "new" behaviour.
eysi09
force-pushed
the
action-log-improvements
branch
from
c8b42ec
to
0e2b4f8
Compare
core/src/tasks/base.ts
Outdated
| willRerun: boolean | ||
| }) { | ||
| if (result.state === "ready") { | ||
| const readyStr = { |
There was a problem hiding this comment.
I feel like this code could use some extracting of those string maps to the outer scope and adding a few line breaks for clarity.
core/src/tasks/base.ts
Outdated
| @@ -492,6 +512,19 @@ export function emitProcessingEvents< | |||
| const actionKind = this.action.kind.toLowerCase() as Lowercase<A["kind"]> | |||
| const eventName = actionKindToEventNameMap[actionKind] | |||
| const startedAt = new Date().toISOString() | |||
| const log = this.log.createLog() | |||
| const version = this.action.versionString() | |||
| const verb = { | |||
There was a problem hiding this comment.
This mapping here should also probably live outside of the function.
Since they're all always using the same indexing over Build, Deploy etc, we can probably do something like
const PROCESSING_EVENTS_LOG_STRINGS = {
Build: {
verb: "Building",
ready: "built",
run: "will be built",
rerun: "re-build",
},
...
}
0e2b4f8
to
09e1b06
Compare
|
Thanks for the review @TimBeyer! I originally co-located these map with the log lines because I kept tweaking it and it made things more visible. But I agree that your suggestion is cleaner, addressed in the latest commit. |
This commits adds more logging to the action lifecycle. That is logs about getting status of a given action, the results, next steps, etc. The logging is done in the task decorators that are also used for emitting events. The goal is obviously to make the logs more informative but also more consistent. Furthermore, this commit fixes the type for the `styles` map which were way off. This means you can "break out" of the map and call all chalk methods when chaining and it would be nice to narrow that down. But we're going for loose and correct for now.
eysi09
force-pushed
the
action-log-improvements
branch
from
09e1b06
to
fc8fc8b
Compare
What this PR does / why we need it:
This commits adds more logging to the action lifecycle. That is logs about getting status of a given action, the results, next steps, etc.
The logging is done in the task decorators that are also used for emitting events.
The goal is obviously to make the logs more informative but also more consistent.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Here's a screenshot of the new logs. Note that the screenshot uses different colors to what we currently have so that the change is more visible. But we'll make those color changes in a separate PR though.