Skip to content

Logs & errors review #1673

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
Merged
ldez merged 1 commit into from
May 30, 2017
Merged

Logs & errors review #1673

ldez merged 1 commit into from
May 30, 2017

Conversation

@ldez
Copy link
Contributor

@ldez ldez commented May 26, 2017 •
edited
Loading

Description

  • log & error: remove format if not necessary, add if necessary.
  • add constants for k8s annotations.
  • fix typos

@ldez ldez added area/logs kind/enhancement a new or improved feature. labels May 26, 2017
@ldez ldez force-pushed the refactor/logs-review branch from 318294b to 29042b9 Compare May 26, 2017 15:33
timoreimann
timoreimann suggested changes May 26, 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.

My OCD ❤️ s this PR. :-)

A few minor remarks, basically just repetitions of two classes of comments.

provider/docker/docker.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Label -> label

provider/docker/docker.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Label -> label

server/configuration.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Bad -> bad

Copy link
Contributor Author

@ldez ldez May 27, 2017

Choose a reason for hiding this comment

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

Why put the first letter in lower case? For me, a sentence begins with an uppercase letter. If it's for consistency, I can change the others too.

Copy link
Contributor

@timoreimann timoreimann May 27, 2017

Choose a reason for hiding this comment

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

Because of this.

Copy link
Contributor Author

@ldez ldez May 27, 2017

Choose a reason for hiding this comment

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

Good to known! 👍

server/configuration.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Bad -> bad

server/configuration.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Bad -> bad

server/rules_test.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Better to use t.Fatalf.

server/rules_test.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Better to use t.Fatalf.

server/rules_test.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Better to use t.Fatalf.

server/rules_test.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Better to use t.Fatalf.

server/rules_test.go Outdated
Copy link
Contributor

@timoreimann timoreimann May 26, 2017

Choose a reason for hiding this comment

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

Better to use t.Fatalf.

@ldez ldez force-pushed the refactor/logs-review branch from fda6eb2 to d54ffbc Compare May 28, 2017 04:37
@ldez ldez added this to the 1.4 milestone May 29, 2017
@ldez ldez force-pushed the refactor/logs-review branch 2 times, most recently from 48baf52 to f12622d Compare May 30, 2017 18:04
emilevauge
emilevauge approved these changes May 30, 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.

Great job @ldez !
LGTM

@ldez
Copy link
Contributor Author

ldez commented May 30, 2017

ping @timoreimann 😉

@ldez
refactor: Logs & errors review.
ad531db 
- log & error: remove format if not necessary, add if necessary.
- add constants for k8s annotations.
- fix typos
@ldez ldez force-pushed the refactor/logs-review branch from f12622d to ad531db Compare May 30, 2017 19:22
timoreimann
timoreimann approved these changes May 30, 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.

Great job, @ldez -- one less consistency problem in the world to think about. 😺 👍

@ldez ldez merged commit cbccdd5 into traefik:master May 30, 2017
@ldez ldez deleted the refactor/logs-review branch May 30, 2017 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emilevauge emilevauge emilevauge approved these changes

+1 more reviewer

@timoreimann timoreimann timoreimann approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/logs kind/enhancement a new or improved feature.

Projects

None yet

Milestone

1.4

Development

Successfully merging this pull request may close these issues.

3 participants

@ldez @timoreimann @emilevauge