checks related fixes #787

unclejack · 2017-03-16T16:34:07Z

This PR makes the following changes:

fixes the issues reported by make checks across the entire repository, except for vendor
removes the exception which excluded the test directory from all tests

This was an issue whenever code was being modified in the test directory. Some files would end up being formatted by gofmt. These gofmt related changes would appear in commits. It's a good idea to follow these rules for the entire code base and not play ping-pong with gofmt when making other changes.

The fixes made to pass checks are the following:

remove punctuation from the end of the errors
make errors start with a lowercase character
gofmt all the code
rename one variable in the tests to pass checks

dseevr

LGTM aside from the capitalization stuff

dseevr · 2017-03-16T17:13:58Z

test/systemtests/bgp_test.go

@@ -932,7 +932,7 @@ func (s *systemtestSuite) CheckBgpConnectionForaNode(c *C, node remotessh.Testbe
 			return nil
 		}
 	}
-	return errors.New("BGP connection not established")
+	return errors.New("bGP connection not established")


This should be all lowercase

dseevr · 2017-03-16T17:14:12Z

test/systemtests/bgp_test.go

@@ -947,7 +947,7 @@ func (s *systemtestSuite) CheckBgpNoConnectionForaNode(c *C, node remotessh.Test
 			return nil
 		}
 	}
-	return errors.New("BGP connection persists")
+	return errors.New("bGP connection persists")


dseevr · 2017-03-16T17:14:42Z

test/systemtests/docker_test.go

@@ -740,15 +740,15 @@ func (d *docker) verifyVTEPs(expVTEPS map[string]bool) (string, error) {
 	for vtep := range expVTEPS {
 		_, found := actVTEPs[vtep]
 		if !found {
-			return str, errors.New("VTEP " + vtep + " not found")
+			return str, errors.New("vTEP " + vtep + " not found")


dseevr · 2017-03-16T17:15:05Z

test/systemtests/k8setup_test.go

@@ -731,15 +731,15 @@ func (k *kubernetes) verifyVTEPs(expVTEPS map[string]bool) (string, error) {
 	for vtep := range expVTEPS {
 		_, found := actVTEPs[vtep]
 		if !found {
-			return str, errors.New("VTEP " + vtep + " not found")
+			return str, errors.New("vTEP " + vtep + " not found")


dseevr · 2017-03-16T17:16:34Z

test/systemtests/swarm_test.go

@@ -698,7 +698,7 @@ func (w *swarm) verifyVTEPs(expVTEPS map[string]bool) (string, error) {
 	for vtep := range expVTEPS {
 		_, found := actVTEPs[vtep]
 		if !found {
-			return str, errors.New("VTEP " + vtep + " not found")
+			return str, errors.New("vTEP " + vtep + " not found")


unclejack · 2017-03-16T19:36:27Z

@dseevr: I've made the changes. PTAL

jojimt

It looks like this PR changes the capitalization of the first letter of a whole bunch of messages from upper case to lower case. It is unclear what value this adds. Can we not do this? It is likely to cause confusion/inconsistency as we print these errors in many paces, in a variety of formats.

jojimt · 2017-03-17T15:06:01Z

drivers/ovsSwitch.go

@@ -95,7 +95,7 @@ func NewOvsSwitch(bridgeName, netType, localIP string, fwdMode string,
 			datapath = "vrouter"
 		default:
 			log.Errorf("Invalid datapath mode")
-			return nil, errors.New("Invalid forwarding mode. Expects 'bridge' or 'routing'")
+			return nil, errors.New("invalid forwarding mode. Expects 'bridge' or 'routing'")


I do not see the point of this change. The original string looks better to me.

The point of this change is that error messages are meant to start with a lowercase letter and not have any punctuation at the end.

The goal of this convention is to be able to compose errors nicely. If you run into an error, you can do something like return fmt.Errorf("encountered error while opening file: %v", err); this error would be composable even further, e.g.: "failed to save data: encountered error while opening file: file doesn't exist".

There are some language conventions which should be followed. Others follow these conventions as well. I didn't come up with these conventions.

This is also the convention that our errored repo follows

https://github.com/pkg/errors is also using lowercase letters for the errors. The static analysis tools we use are used by other repositories as well. This is more of a convention, rather than a personal preference.

dseevr · 2017-05-26T05:12:02Z

@unclejack could you take a quick look at getting this one passing, too? I think this would be worthwhile to get merged as a step towards cleaning up our tests

unclejack · 2017-05-26T14:32:02Z

This is running into some failures for some reason. I've got to look into this.

gkvijay · 2017-05-26T15:57:14Z

This is only changing the error message format. I don't see how that is helping our tests.

unclejack · 2017-05-26T16:03:06Z

@gkvijay: This PR is also removing a blacklist which prevents any kind of code quality tool on the tests themselves. The changes I've made are the ones needed to make it possible to run those tools on the code base. Please feel free to try the other commit on a code checkout to see how make checks fails.

I've had issues in the past where I had to craft PRs just to avoid gofmt related changes because the code wasn't properly formatted. We still have code in tests which isn't properly formatted. Obviously, this can never stop if we have the test directory in the blacklist.

rchirakk · 2017-06-26T17:35:55Z

mgmtfn/k8splugin/driver.go

@@ -122,7 +122,7 @@ func createEP(req *epSpec) (*epAttr, error) {
 	netID := req.Network + "." + req.Tenant
 	ep, err := netdGetEndpoint(netID + "-" + req.EndpointID)
 	if err == nil {
-		return nil, fmt.Errorf("EP %s already exists", req.EndpointID)
+		return nil, fmt.Errorf("the EP %s already exists", req.EndpointID)
 	}


can you change EP-> endpoint ?

rchirakk · 2017-06-26T17:39:52Z

netplugin/cluster/cluster.go

@@ -230,7 +230,7 @@ func masterReq(path string, req interface{}, resp interface{}, isDel bool) error
 	}

 	log.Errorf("Error making POST request. All master failed")
-	return errors.New("POST request failed")
+	return errors.New("the POST request failed")


In general if you are changing a string,
can you make it to more descriptive like "http POST request to master failed "

rchirakk · 2017-06-26T17:46:15Z

@unclejack can you fix the build failure ?
let's merge it in 1.1 or post 1.1

unclejack · 2017-06-26T19:42:22Z

@rchirakk: I've fixed the PR. The CI is running into some issues, but the issue is confirmed to be fixed. Perhaps this should be merged after 1.1. We can include it in 1.1.1.

rchirakk · 2017-06-27T07:52:29Z

mgmtfn/k8splugin/contivk8s/k8s_cni_test.go

@@ -48,7 +48,7 @@ func nsToPID(ns string) (int, error) {
 	// Make sure ns is well formed
 	ok := strings.HasPrefix(ns, "/proc/")
 	if !ok {
-		return -1, fmt.Errorf("Invalid nw name space: %v", ns)
+		return -1, fmt.Errorf("invalid network namespace: %v", ns)
 	}


rchirakk · 2017-06-27T07:56:30Z

test/integration/utils_test.go

@@ -330,7 +330,7 @@ func tcFilterCheckBw(expBw, expBurst int64) error {
 	qdiscoutput := strings.Split(qdiscShow, "ingress")
 	if len(qdiscoutput) < 2 {
 		log.Errorf("Got `tc qdisco show` output:\n%s", qdiscShow)
-		return fmt.Errorf("Unexpected `tc qdisco show` output")
+		return fmt.Errorf("unexpected `tc qdisc show` output")
 	}


no more disco 👍

rchirakk · 2017-06-27T08:09:41Z

core/error.go

 func ErrIfKeyExists(err error) error {
-	if err == nil || strings.Contains(err.Error(), "Key not found") {
+	if err == nil || strings.Contains(err.Error(), "key not found") {
 		return nil


Just to cover all the cases,
better make it independent case, (use string.Tolower(err.Error()) before comparison)

@rchirakk: This was just a quick fix for us to be able to use gofmt, go vet, golint and the other tools on the entire code base. I intend to send some more PRs to refactor the error handling across the code base. I'd rather focus on starting the refactor work to avoid such issues in the future and make our code better.

rchirakk · 2017-06-27T08:10:51Z

LGTM other than the minor question/comment.
@gkvijay @DivyaVavili possible candidate for 1.1.1 ?

unclejack · 2017-07-18T16:02:17Z

Are there any other changes I should make for this? Please let me know.

unclejack · 2017-07-24T14:12:51Z

The 1.1.x release is out.

We need to move forward with this PR.

/cc @DivyaVavili @gkvijay @rchirakk @rhim

unclejack · 2017-08-29T07:50:20Z

Is anyone still interested in this PR?

rhim

LGTM. Minor comments. Once CI is clean, it is OK to merge.

rhim · 2017-08-29T17:43:15Z

mgmtfn/k8splugin/contivk8s/clients/network.go

@@ -86,7 +86,7 @@ func (c *NWClient) AddPod(podInfo interface{}) (*cniapi.RspAddPod, error) {
 		if err != nil {
 			return nil, err
 		}
-		return &data, fmt.Errorf("Internal Server Error")
+		return &data, fmt.Errorf("internal Server Error")


lower case all words?

See the other comments in this PR discussion. This makes error messages composable

rhim · 2017-08-29T17:43:51Z

mgmtfn/k8splugin/driver.go

@@ -122,7 +122,7 @@ func createEP(req *epSpec) (*epAttr, error) {
 	netID := req.Network + "." + req.Tenant
 	ep, err := netdGetEndpoint(netID + "-" + req.EndpointID)
 	if err == nil {
-		return nil, fmt.Errorf("EP %s already exists", req.EndpointID)
+		return nil, fmt.Errorf("the endpoint %s already exists", req.EndpointID)


EP has a specific meaning in contiv, and can be left as in.

rhim · 2017-09-11T17:54:26Z

@unclejack please rebase and resubmit to get it merged in.

Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com>

unclejack · 2017-09-12T19:00:57Z

I've fixed this, but the CI is failing.

dseevr · 2017-09-12T19:03:15Z

Is the k8s CI run expected to fail at this point? I remember there was some work in progress on it...

unclejack · 2017-09-12T20:18:43Z

@dseevr: It should work at this point. The failures are weird.

unclejack · 2017-09-18T10:33:23Z

@dseevr @rhim: This should be good to go now. PTAL

rhim · 2017-09-18T18:29:18Z

ping @contiv/maintainers-core-networking. Please merge this PR.

unclejack · 2017-09-21T12:43:57Z

Is there something else I should do to get this PR merged? Does anyone have any questions or concerns related to this? I'd rather address any concerns now and not require another rebase and test cycle.

rhim · 2017-09-21T15:16:51Z

Nothing further is needed. I will ping maintainers once more.

…

-Himanshu On Sep 21, 2017, at 5:44 AM, unclejack <notifications@github.com<mailto:notifications@github.com>> wrote: Is there something else I should do to get this PR merged? Does anyone have any questions or concerns related to this? I'd rather address any concerns now and not require another rebase and test cycle. - You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#787 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AMiMKduhNAKjk2T-kiKNxpFNPxsPfYCkks5skloQgaJpZM4Mfj3Y>.

dseevr reviewed Mar 16, 2017

View reviewed changes

unclejack force-pushed the checks_fixes branch from a8c353d to 103ecef Compare March 16, 2017 19:16

dseevr approved these changes Mar 16, 2017

View reviewed changes

jojimt requested changes Mar 17, 2017

View reviewed changes

unclejack force-pushed the checks_fixes branch 3 times, most recently from c437e95 to 8ff3c13 Compare March 28, 2017 16:40

unclejack force-pushed the checks_fixes branch 2 times, most recently from 31a3191 to a418f42 Compare May 26, 2017 05:57

unclejack force-pushed the checks_fixes branch from a418f42 to f705d12 Compare June 14, 2017 16:41

unclejack force-pushed the checks_fixes branch 3 times, most recently from 60bcd54 to 81e2f3b Compare June 26, 2017 15:25

rchirakk reviewed Jun 26, 2017

View reviewed changes

unclejack force-pushed the checks_fixes branch from 81e2f3b to b22e046 Compare June 26, 2017 19:16

rchirakk reviewed Jun 27, 2017

View reviewed changes

rhim approved these changes Aug 29, 2017

View reviewed changes

unclejack added 2 commits September 11, 2017 21:50

most packages: fix errors to pass checks

e71883c

Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com>

Makefile: run checks on test as well

c12bcd6

Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com>

unclejack force-pushed the checks_fixes branch from b22e046 to c12bcd6 Compare September 11, 2017 18:50

dseevr approved these changes Sep 18, 2017

View reviewed changes

srampal merged commit a2e4909 into contiv:master Sep 21, 2017

unclejack deleted the checks_fixes branch September 21, 2017 21:18

gkvijay mentioned this pull request Sep 24, 2017

Fixed a golint error in test file #981

Merged

checks related fixes #787

checks related fixes #787

Conversation

unclejack commented Mar 16, 2017

dseevr 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

unclejack commented Mar 16, 2017

jojimt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unclejack Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dseevr commented May 26, 2017

unclejack commented May 26, 2017

gkvijay commented May 26, 2017 • edited Loading

unclejack commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchirakk commented Jun 26, 2017

unclejack commented Jun 26, 2017

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

rchirakk commented Jun 27, 2017

unclejack commented Jul 18, 2017

unclejack commented Jul 24, 2017

unclejack commented Aug 29, 2017

rhim 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

rhim commented Sep 11, 2017

unclejack commented Sep 12, 2017

dseevr commented Sep 12, 2017

unclejack commented Sep 12, 2017

unclejack commented Sep 18, 2017

rhim commented Sep 18, 2017

unclejack commented Sep 21, 2017

rhim commented Sep 21, 2017 via email

unclejack Mar 17, 2017 •

edited

Loading

gkvijay commented May 26, 2017 •

edited

Loading