refactor: new api for dflog and output log to console by default #783
Conversation
inoc603
force-pushed
the
log
branch
2 times, most recently
from
3ccb0ed to
9328e6c
Compare
| opts := []dflog.Option{ | ||
| dflog.WithLogFile(logFilePath), | ||
| dflog.WithSign(fmt.Sprintf("%d", os.Getpid())), | ||
| dflog.WithDebug(options.Debug), |
There was a problem hiding this comment.
Do we also need to make supernode output logs to console as an default option?
There was a problem hiding this comment.
I think so. But currently there's no configuration to disable console log. Should we provide options to turn it off?
inoc603
force-pushed
the
log
branch
3 times, most recently
from
7b0d080 to
8c52158
Compare
Codecov Report
@@ Coverage Diff @@
## master #783 +/- ##
==========================================
+ Coverage 39.41% 39.42% +0.01%
==========================================
Files 108 108
Lines 6310 6288 -22
==========================================
- Hits 2487 2479 -8
+ Misses 3618 3603 -15
- Partials 205 206 +1
Continue to review full report at Codecov.
|
|
|
||
| // open log file | ||
| logFile, err := os.OpenFile(logFilePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644) |
There was a problem hiding this comment.
It's better to specify the FileMode as 0644, all users can read the log file. If not, the created file's mode is relied on the os's umask. If the os's umask is 027, then the new file's mode is 0640, it means only the owner and users in same group can read the file.
There was a problem hiding this comment.
The version of lumberjack I'm using, v2.0.0 did set the file mode to 0644. However in this commit it changes the default mode to 0600. I've been using this library in production for quite a while, and it's pretty stable. If 0644 is what we want, we can keep using v2.0.0. If there's an update in the future, we can create a PR to make the default mode configurable.
There was a problem hiding this comment.
OK. I find that the previous code has same problem with this pr. If we want to create a file with permission 0644 exactly, we should explicitly call syscall.Umask to set umask.
Signed-off-by: inoc603 <[email protected]>
|
LGTM |
* docs(zh-CN): remove README.md machine translation Signed-off-by: zzy987 <[email protected]> * docs: fixed some machine translation Signed-off-by: zzy987 <[email protected]> * fix: some translation Signed-off-by: zzy987 <[email protected]> * chore: update helm-charts Signed-off-by: zzy987 <[email protected]> * doc: markdown lint Signed-off-by: zzy987 <[email protected]> * doc: markdown lint Signed-off-by: zzy987 <[email protected]> * doc: markdown lint Signed-off-by: zzy987 <[email protected]>
Signed-off-by: inoc603 [email protected]
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
fixes #651
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Ⅳ. Describe how to verify it
Start dfdaemon and check the console output
Ⅴ. Special notes for reviews