iframe-proxy

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

 		}
 	}
-	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

 		}
 	}
-	return errors.New("BGP connection persists")
+	return errors.New("bGP connection persists")


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

 		_, 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

 		_, 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

 		_, 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

 		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

 	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


 	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

 	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

 		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

 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

 			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

 	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

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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

unclejack commented Mar 16, 2017

Uh oh!

dseevr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unclejack commented Mar 16, 2017

Uh oh!

jojimt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unclejack Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dseevr commented May 26, 2017

Uh oh!

unclejack commented May 26, 2017

Uh oh!

gkvijay commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unclejack commented May 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchirakk commented Jun 26, 2017

Uh oh!

unclejack commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchirakk commented Jun 27, 2017

Uh oh!

unclejack commented Jul 18, 2017

Uh oh!

unclejack commented Jul 24, 2017

Uh oh!

unclejack commented Aug 29, 2017

Uh oh!

rhim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhim commented Sep 11, 2017

Uh oh!

unclejack commented Sep 12, 2017

Uh oh!

unclejack Mar 17, 2017 •

edited

Loading

gkvijay commented May 26, 2017 •

edited

Loading