- Notifications
You must be signed in to change notification settings - Fork1k
feat(cli): rotate file logs for coderd#15438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, but want to ensure we have like-for-like behaviour with the agent.
| } | ||
| closers=append(closers,fi.Close) | ||
| sinks=append(sinks,sinkFn(fi)) | ||
| closers=append(closers,logWriter.Close) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this give us the equivalent behaviour oflumberjackWriteCloseFixer incli/agent.go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should probably re-use that same fix here, although the main reason for that seems to have been to reduce CI flakes due to racy writes after close. We probably won't be logging to a file in CI in the general case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See inline comment below; bonus point for a test in./cli but I'm not going to insist on it for this PR.
Agreed on a test; I was thinking about this over the weekend. |
johnstcn left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I smoke-tested this locally and noted our existing test coverage inTestServer/Logging.*. Testing Lumberjack's rotation is probably way too much overhead here.
If we wanted to add a test, we could extractaddSinkIfNotProvided() incli/clilog and test that on its own.
I trust your judgment. I'm happy to merge as-is. |
d6442db intomainUh oh!
There was an error while loading.Please reload this page.
Related to#15309
As we already are doing for agent logs - this PR is enabling the logs rotation for coderd logs.
Currently keeping the same logic than we had for agent - with 5MB as the file size for rotation.