prReviewBestPractices

Here are some PR review practices I follow. The list can be extended, as I'm sure every developer on the team has a unique perspective that can be useful to others.

So you got the PR for review. There are a couple of logistical things to verify first:

* Make sure there is an issue linked to the PR. 
* If the PR a bug fix, make sure the issue has clear steps to reproduce and validate. It's a good idea to include a quick summary to the pr itself. Good example: https://github.com/rancher/rke/pull/1752#issue-336681310
* If the PR is a new feature or enhancemenet, there should be a functional spec/design doc listing all the use cases.
* For bigger features, it makes sense to reach out to feature engineer(s) so they can walk you throug the high level logic

After you understood the feature/fix speficis, it's time to start the actual code review. Here are several items I pay particular attention to in a context of Rancher PR code reviews:

* If you are introducing a new controller:
- Check if it can be placed into user cluster context vs management context. It would have great performance benefits in setups having many clusters
- Use listers (cache) vs pulling the object from API, whenever possible. It will minimize number of hits on the etcd database.
- Use sync() vs lifecyle() in cases when cleanup on object removal is not necessary. Lifecycle is heavy on updates, and uses finalizers.
- Don't log errors in controllers if you return the error to the framework in sync/lifecycle methods. The framework will automatically log any error returned to it.

* If you span a new go routine, make sure to pass the context to it. This way if Rancher exits, all the independent go routines will be canceled along. Example: https://github.com/rancher/rancher/blob/48752a0a214008c9ee556d2935c1c3ceb1b46ab1/pkg/controllers/management/catalog/controller.go#L102
* Use norman/wragler/rancherUtils framework methods whenever possible, instead of writing stuff from scratch. Review similar code to get an idea of what methods can be used. Examples:
- github.com/rancher/rancher/pkg/ticker.Context
- github.com/rancher/norman/types/convert
- github.com/rancher/norman/types/mapper
- github.com/rancher/norman/types/values

* If your code updates some object in etcd, make sure to do the update only when the new and old objects are different. That is to 
minimize number of unnecessary hits on etcd. Never update object passed to you by sync/lifecycle directly; always do a deepCopy

* If it's an API change, always make sure backwards compatibility is not broken. Good example: changing the type of existing field
is not recommended.
* Make sure you add enough logging. It makes a world of a diff to support when it comes to debugging. If you feel that logging in INFO would be too verbose, logging in DEBUG is a good alternative.

Code formatting review pracices I follow:

* If method is too long, see if you can split it into several ones.
* Reuse the code if possible. Use the constants instead of using hardcoded values.