Create log folder if not present #1507
Conversation
middlewares/logger.go
Outdated
|
|
||
| err := os.MkdirAll(dir, 0755) | ||
| if err != nil { | ||
| log.Error("Error making directories ", err) |
There was a problem hiding this comment.
Can we mention the log directory like
"Failed to create log path %s: %s", dir, err
middlewares/logger_test.go
Outdated
| @@ -20,7 +20,7 @@ type logtestResponseWriter struct{} | |||
|
|
|||
| var ( | |||
| logger *Logger | |||
| logfileName = "traefikTestLogger.log" | |||
| logfileName = "/traefik/logger/test.log" | |||
There was a problem hiding this comment.
Could you rename this to logfileNameSuffix since it will be concatenated with the actual temp directory.
There was a problem hiding this comment.
Could you please also remove the global variable logfilePath? It suffices to live on the test function stack.
|
Thank you for your feedbacks @timoreimann |
|
Apparently, log files are now created at two locations. Is that necessary? Aren't those the same log files? |
|
I'm afraid not. One is traefik log file, the other is access log file. |
middlewares/logger_test.go
Outdated
| logfilePath = filepath.Join("/tmp", logfileName) | ||
| tmp, err := ioutil.TempDir("", "testlogger") | ||
| if err != nil { | ||
| t.Fatal("failed to create temp dir: ", err) |
There was a problem hiding this comment.
Can we use Fatalf and %s just for consistency reasons?
middlewares/logger_test.go
Outdated
| defer logger.Close() | ||
|
|
||
| if _, err := os.Stat(logfilePath); err != nil { | ||
| t.Fatalf("logger should create %q", logfilePath) |
There was a problem hiding this comment.
Any particular reason we're using %q here as opposed to just %s?
| if err != nil { | ||
| log.Errorf("Failed to create log path %s: %s", dir, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
The file seems completely untested, but making it properly testable is likely a bigger refactoring we want to source out to a different PR.
Could you verify manually that this part of the code change works as expected please?
middlewares/logger_test.go
Outdated
| defer cleanup() | ||
| defer logger.Close() | ||
|
|
||
| if _, err := os.Stat(logfilePath); err != nil { |
There was a problem hiding this comment.
I think you need os.IsNotExist, don't you?
tanyadegurechaff
force-pushed
the
create-log-folder
branch
from
6ff3b0f
to
67154a0
Compare
|
CI flaked out -- retriggered. |
|
@emilevauge @ldez looks simple enough to still get into 1.3, WDYT? |
ldez
added
contributor/needs-resolve-conflicts
kind/bug/fix
|
@timoreimann why not, my biggest concern is that this PR #1485 is a big refactor of the logger and we will probably not be able to merge the current one once #1485 is merged. WDYT @tanyadegurechaff ? |
|
@emilevauge this change is rather smallish. My assumption is that it should be easy for #1485 to adjust / fix a smallish merge commit. @rjshep, any thoughts? |
|
@emilevauge put differently, I'd rather have the big change pay an integration effort for a small change than the other way around. Or is it that #1485 does things so fundamentally different as far as this PR is concerned that we will run into bigger problems I don't see right now? |
ldez
removed
priority/P1
|
Due to SemaphoreCI, I close and reopen the PR. |
Description
Fixes #1217