Feature/logfile #8
Conversation
Added a flag to define a log file that saves the log seperatly. Signed-off-by: llogen <[email protected]>
Added comment. Signed-off-by: llogen <[email protected]>
|
at a glance, the test failure doesnt seem related, but just to make sure @9eCyberSec can you run ./run_tests.sh locally and see if it fails in the same way? You should just need docker (+compose) as deps |
|
Locally it just works fine. |
|
i've no idea why github didnt send any notifications for your last message, so sorry about that; i'll pull the branch and see why that test is failing |
|
ok, so i ran this a couple of times over the last 2 weeks and it does fail sometimes with timeout. There have been some fixes for tests in the meantime, could you try to rebase on latest and push? Thanks |
Added a flag to define a log file that saves the log seperatly. Signed-off-by: llogen <[email protected]>
… feature/logfile Signed-off-by: llogen <[email protected]>
|
Thank you for your answer, i did the rebase. Hope it works now. |
Codecov Report
@@ Coverage Diff @@
## main #8 +/- ##
=======================================
Coverage ? 66.14%
=======================================
Files ? 156
Lines ? 9347
Branches ? 0
=======================================
Hits ? 6183
Misses ? 2498
Partials ? 666
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
cool, the tests run now; just need to signoff for the DCO |
|
seems this didnt get any updates since last year; i'll comandeer later on and merge |
9eCyberSec
dismissed
mimir-d’s stale review
The merge-base changed after approval.
| return err | ||
| } | ||
| defer f.Close() | ||
| log.OriginalLogger().(*logrus.Entry).Logger.SetOutput(io.MultiWriter(os.Stderr, f)) |
There was a problem hiding this comment.
@xaionaro does this make sense here? i dont know if we actually use logrus anymore
There was a problem hiding this comment.
i dont know if we actually use logrus anymore
At least the upstream version of contest uses logrus:
https://github.com/linuxboot/contest/blob/develop/pkg/logging/default_options.go#L25
does this make sense here?
It kinda does. But a cleaner solution would be to feed an option to https://github.com/linuxboot/contest/blob/develop/pkg/logging/default_options.go#L21-L24 to add a multiwriter there if needed (to avoid type assertions; or at least to keep them local to where we make sure it is logrus). If one wants to avoid a type assertion at all then they can manually initialize a logrus instance and wrap it using this function: https://github.com/facebookincubator/go-belt/blob/main/tool/logger/implementation/logrus/logger.go#L484
Or if people are lazy, they can just move this line into the function WithBelt (to keep the assertion local to where we build a logrus logger).
There was a problem hiding this comment.
yeah, that's what I was thinking. that type assertion looks kinda off to me. Not sure what we can do here, because this PR comes from a 9elements repo so we can't fix anything on this branch without the original author
Added a flag to define a log file that saves the log seperatly.