Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create log folder if not present #1507

Merged
merged 1 commit into from May 19, 2017
Merged

Create log folder if not present #1507

merged 1 commit into from May 19, 2017

Conversation

tanyadegurechaff
Copy link
Contributor

Description

Fixes #1217

@timoreimann timoreimann self-requested a review April 28, 2017 05:50
@ldez ldez added the kind/enhancement a new or improved feature. label Apr 28, 2017
timoreimann
timoreimann suggested changes Apr 28, 2017
View reviewed changes
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more things:

  1. We are yet missing a test assertion. Could you add a check verifying that the file exists after we have created the logger?
  2. Please replace the hand-crafted determination of the platform-dependent temporary folder by a call to os.TempDir.

middlewares/logger.go Outdated

err := os.MkdirAll(dir, 0755)
if err != nil {
log.Error("Error making directories ", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this to logfileNameSuffix since it will be concatenated with the actual temp directory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also remove the global variable logfilePath? It suffices to live on the test function stack.

@tanyadegurechaff
Copy link
Contributor Author

Thank you for your feedbacks @timoreimann

@timoreimann
Copy link
Contributor

Apparently, log files are now created at two locations. Is that necessary? Aren't those the same log files?

@tanyadegurechaff
Copy link
Contributor Author

I'm afraid not. One is traefik log file, the other is access log file.

timoreimann
timoreimann suggested changes Apr 29, 2017
View reviewed changes
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason we're using %q here as opposed to just %s?

cmd/traefik/traefik.go
if err != nil {
log.Errorf("Failed to create log path %s: %s", dir, err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

middlewares/logger_test.go Outdated
defer cleanup()
defer logger.Close()

if _, err := os.Stat(logfilePath); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need os.IsNotExist, don't you?

timoreimann
timoreimann approved these changes Apr 29, 2017
View reviewed changes
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Could you squash please?

@timoreimann
Copy link
Contributor

CI flaked out -- retriggered.

@timoreimann
Copy link
Contributor

@emilevauge @ldez looks simple enough to still get into 1.3, WDYT?

@ldez ldez added contributor/needs-resolve-conflicts kind/bug/fix a bug fix and removed kind/enhancement a new or improved feature. labels Apr 29, 2017
@emilevauge
Copy link
Member

@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 ?

@ldez ldez added the priority/P1 need to be fixed in next release label Apr 29, 2017
@timoreimann
Copy link
Contributor

@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?

@timoreimann
Copy link
Contributor

@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 ldez added the area/logs label May 9, 2017
@ldez ldez modified the milestone: 1.4 May 10, 2017
@ldez ldez removed priority/P1 need to be fixed in next release contributor/needs-resolve-conflicts labels May 10, 2017
@ldez ldez modified the milestones: 1.3, 1.4 May 19, 2017
@ldez ldez changed the base branch from master to v1.3 May 19, 2017 13:07
@ldez
Copy link
Member

ldez commented May 19, 2017

Due to SemaphoreCI, I close and reopen the PR.

@ldez ldez closed this May 19, 2017
@ldez ldez reopened this May 19, 2017
emilevauge
emilevauge approved these changes May 19, 2017
View reviewed changes
Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tanyadegurechaff !
LGTM

@emilevauge emilevauge merged commit 987ae92 into traefik:v1.3 May 19, 2017
@tanyadegurechaff tanyadegurechaff deleted the create-log-folder branch May 19, 2017 13:52
@rjshep rjshep mentioned this pull request May 21, 2017
Merged
@ldez ldez mentioned this pull request Jun 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timoreimann timoreimann timoreimann approved these changes

@emilevauge emilevauge emilevauge approved these changes

Assignees
No one assigned
Labels
area/logs kind/bug/fix a bug fix
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tanyadegurechaff @timoreimann @emilevauge @ldez