feat: add OutputFile option for log destination in logger#152
feat: add OutputFile option for log destination in logger#152
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.
- Relax file permissions from 0600 to 0644 to allow reads (per Albert's review) - Track open log file handle in package-level state protected by mutex to prevent FD leaks when ApplyOptionsToLoggers is called multiple times - Close previous log file before opening a new one on re-apply - Revert output to stdout when OutputFile is empty (handles config reload) - Extract setLogOutput helper for clarity - Add TestApplyOptionsToLoggersFileOutputReapply test verifying re-apply closes the previous file and switches to the new one Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
|
Follow-up on the review feedback:
Latest commit with the lint follow-up: Could you please take another look when you have a moment? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Open new log file before closing the old one so loggers are never left pointing at a closed FD if OpenFile fails - Register t.Cleanup immediately after ApplyOptionsToLoggers to prevent FD leaks on assertion failures Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
|
@JoshVanL The file handle is tracked and closed — see
// Close the previous log file after loggers have been redirected.
if logOutputFile != nil {
logOutputFile.Close()
}
logOutputFile = newFileWhen |
Adds OutputFile option to logger and a --log-file CLI flag. When set, all registered loggers write to the specified file instead of stdout. - Mutex-protected package-level state tracks the active log file - Atomic swap: new file opens before old one closes (no FD leak) - Output reverts to stdout when OutputFile is empty Backport of dapr#152 onto v0.16.1 for D3E release-1.16. Tracks: OSS-1226 Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
* feat: add OutputFile option for log destination in logger Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * feat: add file output support for logging Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * lint Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * add cleanup for file output logger in options tests Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * fix: address review feedback for log file output - Relax file permissions from 0600 to 0644 to allow reads (per Albert's review) - Track open log file handle in package-level state protected by mutex to prevent FD leaks when ApplyOptionsToLoggers is called multiple times - Close previous log file before opening a new one on re-apply - Revert output to stdout when OutputFile is empty (handles config reload) - Extract setLogOutput helper for clarity - Add TestApplyOptionsToLoggersFileOutputReapply test verifying re-apply closes the previous file and switches to the new one Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> * fix: improve error handling and output file management in logging Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * fix: address Copilot review — atomic file swap and early test cleanup - Open new log file before closing the old one so loggers are never left pointing at a closed FD if OpenFile fails - Register t.Cleanup immediately after ApplyOptionsToLoggers to prevent FD leaks on assertion failures Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> --------- Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> Co-authored-by: Nelson Parente <nelson_parente@live.com.pt>
…elease-0.17 (#153) * fix: support RSA and fix ed25519 key encoding in EncodePrivateKey (#148) * fix: support RSA key encoding in EncodePrivateKey EncodePrivateKey only handled *ecdsa.PrivateKey and ed25519.PrivateKey, rejecting RSA keys with "unsupported key type". Since RSA keys also use x509.MarshalPKCS8PrivateKey with the same PKCS#8 block type, add *rsa.PrivateKey to the existing case list. Adds table-driven tests covering RSA-2048, RSA-4096, ECDSA P-256, Ed25519, and unsupported type with roundtrip verification through DecodePEMPrivateKey. Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> * fix: address review feedback - Drop RSA-4096 test case to avoid slow CI (2048 is sufficient coverage) - Replace custom containsSubstr/searchSubstr with strings.Contains - Remove unused helper functions Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> --------- Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> * feat: add OutputFile option for log destination in logger (#152) * feat: add OutputFile option for log destination in logger Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * feat: add file output support for logging Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * lint Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * add cleanup for file output logger in options tests Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * fix: address review feedback for log file output - Relax file permissions from 0600 to 0644 to allow reads (per Albert's review) - Track open log file handle in package-level state protected by mutex to prevent FD leaks when ApplyOptionsToLoggers is called multiple times - Close previous log file before opening a new one on re-apply - Revert output to stdout when OutputFile is empty (handles config reload) - Extract setLogOutput helper for clarity - Add TestApplyOptionsToLoggersFileOutputReapply test verifying re-apply closes the previous file and switches to the new one Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> * fix: improve error handling and output file management in logging Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> * fix: address Copilot review — atomic file swap and early test cleanup - Open new log file before closing the old one so loggers are never left pointing at a closed FD if OpenFile fails - Register t.Cleanup immediately after ApplyOptionsToLoggers to prevent FD leaks on assertion failures Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> --------- Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> Co-authored-by: Nelson Parente <nelson_parente@live.com.pt> --------- Signed-off-by: Nelson Parente <nelson_parente@live.com.pt> Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> Co-authored-by: Mirel <15373565+MyMirelHub@users.noreply.github.com>
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: