feat: add OutputFile option for log destination in logger#152
feat: add OutputFile option for log destination in logger#152MyMirelHub wants to merge 4 commits intodapr:mainfrom
Conversation
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds support for directing logger output to a user-specified file via a new option/flag, and validates the behavior with unit tests.
Changes:
- Introduces
Options.OutputFileand registers a new--log-fileCLI flag. - Updates
ApplyOptionsToLoggersto open the configured file and route logger output to it. - Adds tests covering default values, flag registration, and file-output behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
logger/options.go |
Adds OutputFile option + --log-file flag and applies file output in ApplyOptionsToLoggers. |
logger/options_test.go |
Extends options/flag tests and adds a test that verifies logs are written to the configured file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if options.OutputFile != "" { | ||
| file, err := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open log file %q: %w", options.OutputFile, err) | ||
| } | ||
|
|
||
| for _, v := range internalLoggers { | ||
| v.SetOutput(file) | ||
| } | ||
| } |
There was a problem hiding this comment.
ApplyOptionsToLoggers opens the configured log file and sets it as output for all loggers, but the file handle is never tracked/closed. If options are re-applied (eg during a reload) this will leak file descriptors, and clearing/changing OutputFile won’t revert outputs or close the previous file. Consider storing the current output writer/file in package-level state (protected by a mutex), closing/replacing it on subsequent calls, and explicitly resetting output back to the default (stdout) when OutputFile is empty.
| } | ||
|
|
||
| if options.OutputFile != "" { | ||
| file, err := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600) |
There was a problem hiding this comment.
Should we relax permissions a bit and allow reads?
| file, err := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600) | |
| file, err := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) |
Description
This PR adds support for writing logger output to a file.
Changes made:
OutputFileto logger options and initialized it in defaults.--log-fileCLI flag inAttachCmdFlags.ApplyOptionsToLoggersto open the configured file and route logger output to it.Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: