Logs & errors review #1673

ldez · 2017-05-26T15:05:41Z

Description

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

timoreimann

My OCD ❤️ s this PR. :-)

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

timoreimann · 2017-05-26T23:24:55Z

provider/docker/docker.go

@@ -705,7 +705,7 @@ func getLabel(container dockerData, label string) (string, error) {
 			return value, nil
 		}
 	}
-	return "", errors.New("Label not found:" + label)
+	return "", fmt.Errorf("Label not found: %s", label)


Label -> label

timoreimann · 2017-05-26T23:25:03Z

provider/docker/docker.go

@@ -715,7 +715,7 @@ func getLabels(container dockerData, labels []string) (map[string]string, error)
 		foundLabel, err := getLabel(container, label)
 		// Error out only if one of them is defined.
 		if err != nil {
-			globalErr = errors.New("Label not found: " + label)
+			globalErr = fmt.Errorf("Label not found: %s", label)


Label -> label

timoreimann · 2017-05-26T23:26:10Z

server/configuration.go

@@ -88,7 +88,7 @@ func (dep *DefaultEntryPoints) String() string {
 func (dep *DefaultEntryPoints) Set(value string) error {
 	entrypoints := strings.Split(value, ",")
 	if len(entrypoints) == 0 {
-		return errors.New("Bad DefaultEntryPoints format: " + value)
+		return fmt.Errorf("Bad DefaultEntryPoints format: %s", value)


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.

Because of this.

Good to known! 👍

timoreimann · 2017-05-26T23:26:17Z

server/configuration.go

@@ -127,7 +127,7 @@ func (ep *EntryPoints) Set(value string) error {
 	regex := regexp.MustCompile("(?:Name:(?P<Name>\\S*))\\s*(?:Address:(?P<Address>\\S*))?\\s*(?:TLS:(?P<TLS>\\S*))?\\s*((?P<TLSACME>TLS))?\\s*(?:CA:(?P<CA>\\S*))?\\s*(?:Redirect.EntryPoint:(?P<RedirectEntryPoint>\\S*))?\\s*(?:Redirect.Regex:(?P<RedirectRegex>\\S*))?\\s*(?:Redirect.Replacement:(?P<RedirectReplacement>\\S*))?\\s*(?:Compress:(?P<Compress>\\S*))?")
 	match := regex.FindAllStringSubmatch(value, -1)
 	if match == nil {
-		return errors.New("Bad EntryPoints format: " + value)
+		return fmt.Errorf("Bad EntryPoints format: %s", value)


timoreimann · 2017-05-26T23:26:24Z

server/configuration.go

@@ -314,7 +314,7 @@ func (certs *Certificates) Set(value string) error {
 	for _, certificate := range certificates {
 		files := strings.Split(certificate, ",")
 		if len(files) != 2 {
-			return errors.New("Bad certificates format: " + value)
+			return fmt.Errorf("Bad certificates format: %s", value)


timoreimann · 2017-05-26T23:27:25Z

server/rules_test.go

@@ -41,23 +40,21 @@ func TestParseTwoRules(t *testing.T) {
 	routeResult, err := rules.Parse(expression)

 	if err != nil {
-		t.Fatal("Error while building route for Host:foo.bar;Path:/FOObar")
+		t.Fatal("Error while building route for Host:foo.bar;Path:/FOObar", err)


Better to use t.Fatalf.

timoreimann · 2017-05-26T23:27:28Z

server/rules_test.go

 	}

 	request, err := http.NewRequest("GET", "http://foo.bar/foobar", nil)
 	routeMatch := routeResult.Match(request, &mux.RouteMatch{Route: routeResult})

 	if routeMatch == true {
-		t.Log(err)
-		t.Fatal("Rule Host:foo.bar;Path:/FOObar don't match")
+		t.Fatal("Rule Host:foo.bar;Path:/FOObar don't match", err)


Better to use t.Fatalf.

timoreimann · 2017-05-26T23:27:32Z

server/rules_test.go

 	}

 	request, err = http.NewRequest("GET", "http://foo.bar/FOObar", nil)
 	routeMatch = routeResult.Match(request, &mux.RouteMatch{Route: routeResult})

 	if routeMatch == false {
-		t.Log(err)
-		t.Fatal("Rule Host:foo.bar;Path:/FOObar don't match")
+		t.Fatal("Rule Host:foo.bar;Path:/FOObar don't match", err)


Better to use t.Fatalf.

timoreimann · 2017-05-26T23:27:37Z

server/rules_test.go

@@ -92,48 +89,48 @@ func TestPriorites(t *testing.T) {
 	rules := &Rules{route: &serverRoute{route: router.NewRoute()}}
 	routeFoo, err := rules.Parse("PathPrefix:/foo")
 	if err != nil {
-		t.Fatal("Error while building route for PathPrefix:/foo")
+		t.Fatal("Error while building route for PathPrefix:/foo", err)


Better to use t.Fatalf.

timoreimann · 2017-05-26T23:27:42Z

server/rules_test.go

 	}

 	multipleRules := &Rules{route: &serverRoute{route: router.NewRoute()}}
 	routeFoobar, err := multipleRules.Parse("PathPrefix:/foobar")
 	if err != nil {
-		t.Fatal("Error while building route for PathPrefix:/foobar")
+		t.Fatal("Error while building route for PathPrefix:/foobar", err)


Better to use t.Fatalf.

emilevauge

Great job @ldez !
LGTM

ldez · 2017-05-30T19:13:55Z

ping @timoreimann 😉

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

timoreimann

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

ldez added area/logs kind/enhancement a new or improved feature. labels May 26, 2017

ldez force-pushed the refactor/logs-review branch from 318294b to 29042b9 Compare May 26, 2017 15:33

ldez added the status/2-needs-review label May 26, 2017

timoreimann suggested changes May 26, 2017

View reviewed changes

ldez force-pushed the refactor/logs-review branch from fda6eb2 to d54ffbc Compare May 28, 2017 04:37

ldez added this to the 1.4 milestone May 29, 2017

ldez force-pushed the refactor/logs-review branch 2 times, most recently from 48baf52 to f12622d Compare May 30, 2017 18:04

emilevauge approved these changes May 30, 2017

View reviewed changes

refactor: Logs & errors review.

ad531db

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

ldez force-pushed the refactor/logs-review branch from f12622d to ad531db Compare May 30, 2017 19:22

timoreimann approved these changes May 30, 2017

View reviewed changes

ldez merged commit cbccdd5 into traefik:master May 30, 2017

ldez deleted the refactor/logs-review branch May 30, 2017 21:33

ldez removed the status/2-needs-review label Jun 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logs & errors review #1673

Logs & errors review #1673

ldez commented May 26, 2017 •

edited

Loading

timoreimann left a comment

timoreimann May 26, 2017

timoreimann May 26, 2017

timoreimann May 26, 2017

ldez May 27, 2017

timoreimann May 27, 2017

ldez May 27, 2017

timoreimann May 26, 2017

timoreimann May 26, 2017

timoreimann May 26, 2017

timoreimann May 26, 2017

timoreimann May 26, 2017

timoreimann May 26, 2017

timoreimann May 26, 2017

emilevauge left a comment

ldez commented May 30, 2017

timoreimann left a comment

Logs & errors review #1673

Logs & errors review #1673

Conversation

ldez commented May 26, 2017 • edited Loading

Description

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

ldez commented May 30, 2017

timoreimann left a comment

Choose a reason for hiding this comment

ldez commented May 26, 2017 •

edited

Loading