Add CI log grouping for align-deps output#4150
Conversation
vivekjm
requested review from
JasonVMo,
Saadnajmi,
acoates-ms and
tido64
as code owners
vivekjm
changed the title
tido64
left a comment
tido64
left a comment
There was a problem hiding this comment.
Thanks for your contribution! I've left some comments.
|
|
||
| export function logGroupFor( | ||
| title: string, | ||
| env: Env = process.env |
There was a problem hiding this comment.
Nit: Type can be inferred
| env: Env = process.env | |
| env = process.env |
There was a problem hiding this comment.
Good call, thanks. I removed the explicit env type and let TypeScript infer it from the default value.
| ): LogGroup | undefined { | ||
| if (env["GITHUB_ACTIONS"] === "true") { | ||
| return { | ||
| start: `::group::${escapeGitHubCommand(title)}`, |
There was a problem hiding this comment.
We should use encodeURI instead:
| start: `::group::${escapeGitHubCommand(title)}`, | |
| start: `::group::${encodeURI(title)}`, |
There was a problem hiding this comment.
Done. I switched the GitHub Actions group title handling to use encodeURI here.
| // disk only when everything is in order for the target package. Packages with | ||
| // invalid or missing configurations are skipped. | ||
| const errors = manifests.reduce((errors, manifest) => { | ||
| const logGroup = logGroupFor(manifest); |
There was a problem hiding this comment.
Can we always return start/end functions instead?
const { beginGroup, endGroup } = getGroupMarkers();
const errors = manifests.reduce((errors, manifest) => {
try {
beginGroup(manifest);
...
} finally {
endGroup(manifest);
}There was a problem hiding this comment.
Updated this to expose beginGroup/endGroup through getGroupMarkers. I also moved the call site behind a small helper so the CLI loop stays readable.
| if (logGroup) { | ||
| console.log(logGroup.start); | ||
| } |
There was a problem hiding this comment.
I don't think this is correct. This will always start a group even though we don't know if anything will be logged yet. When no changes are needed, nothing should be logged.
There was a problem hiding this comment.
Agreed. I changed the implementation so the group starts lazily on the first log/warn/error for that manifest. If nothing is logged, no group markers are emitted.
| }; | ||
| } | ||
|
|
||
| export function withLogGroup<T>( |
There was a problem hiding this comment.
I think the withLogGroup pattern is a good one. However, I don't think that we should be overwriting global.console like this. I created a draft of what I had in mind here: #4156
If you don't mind, we can close this one in favour of that one. I'll give you full credit for it, of course.
There was a problem hiding this comment.
yes please !! feel free to close it
|
Superseded by #4156 |
2 participants
Summary
Adds CI log grouping around
align-depsoutput so runs over larger workspaces are easier to scan in GitHub Actions and Azure DevOps.This keeps local output unchanged and only opens a group when a package actually writes log output, avoiding empty groups for packages with no changes.
Fixes #2906.
Testing
git diff --checkyarn workspace @rnx-kit/align-deps formatPATH="/Users/vivek/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:$PATH" yarn workspace @rnx-kit/align-deps lintPATH="/Users/vivek/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:$PATH" yarn workspace @rnx-kit/align-deps build --dependenciesPATH="/Users/vivek/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:$PATH" yarn workspace @rnx-kit/align-deps test