Add contributor and reviewer expectations

standards.md

@@ -1,47 +1,134 @@
-# Tekton Development Standards
+# Tekton Pipelines Contributor and Reviewer Expectations
 
-This doc contains the standards we try to uphold across all project
-in the Tekton org.
+The purpose of this doc is to:
 
-## Principles
+* Outline for contributors what we expect to see in a pull request
+* Establish a baseline for reviewers so they know at a minimum what to look for
 
-When possbile, try to practice:
+Design: Most designs should be first proposed via an issue and possibly a
+[TEP](https://github.com/tektoncd/community/tree/master/teps#tekton-enhancement-proposals-teps).
+API changes should be evaluated according to
+[Tekton Design Principles](https://github.com/tektoncd/community/blob/master/design-principles.md).
 
-- **Documentation driven development** - Before implementing anything, write
-  docs to explain how it will work
-- **Test driven development** - Before implementing anything, write tests to
-  cover it
+Each Pull Request is expected to meet the following expectations around:
 
-Minimize the number of integration tests written and maximize the unit tests!
-Unit test coverage should increase or stay the same with every PR.
+* [Pull Request Description](#pull-request-description)
+* [Commits](#commits)
+* [Docs](#docs)
+* [Functionality](#functionality)
+* [Content](#content)
+* [Code](#code)
+  * [Tests](#tests)
+  * [Reconciler/Controller Changes](#reconcilercontroller-changes)
 
-This means that most PRs should include both:
+_See also [the Tekton review process](https://github.com/tektoncd/community/blob/master/process.md#reviews)._
 
-1. Tests
-2. Documentation updates
+## Pull request description
 
-## Commit Messages
+* Include a link to the issue being addressed, but describe the context for the reviewer
+  * If there is no issue, consider whether there should be one:
+    * New functionality must be designed and approved, may require a TEP
+    * Bugs should be reported in detail
+  * If the template contains a checklist, it should be checked off
+  * Release notes filled in for user visible changes (bugs + features),
+    or removed if not applicable (refactoring, updating tests) (may be enforced
+    via the [release-note Prow plugin](https://github.com/tektoncd/plumbing/blob/master/prow/plugins.yaml))
 
-All commit messages should follow
-[these best practices](https://chris.beams.io/posts/git-commit/), specifically:
+## Commits
 
-- Start with a subject line
-- Contain a body that explains _why_ you're making the change you're making
-- Reference an issue number one exists, closing it if applicable (with text such
-  as
-  ["Fixes #245" or "Closes #111"](https://help.github.com/articles/closing-issues-using-keywords/))
+* Follow [commit messages best practices](https://chris.beams.io/posts/git-commit/):
+  1. Separate subject from body with a blank line
+  2. Limit the subject line to 50 characters
+  3. Capitalize the subject line
+  4. Do not end the subject line with a period
+  5. Use the imperative mood in the subject line
+  6. Wrap the body at 72 characters
+  7. Use the body to explain what and why vs. how. Don't just link to an issue and
+      [aim for 2 paragraphs](https://www.youtube.com/watch?v=PJjmw9TRB7s), e.g.:
+      * What is the problem being solved?
+      * Why is this the best approach?
+      * What other approaches did you consider?
+      * What side effects will this approach have?
+      * What future work remains to be done?
+* Prefer one commit per PR; if there are multiple commits, ensure each is
+  self-contained and makes sense without the context of the others in the PR
 
-Aim for [2 paragraphs in the body](https://www.youtube.com/watch?v=PJjmw9TRB7s).
-Not sure what to put? Include:
+## Docs
 
-- What is the problem being solved?
-- Why is this the best approach?
-- What other approaches did you consider?
-- What side effects will this approach have?
-- What future work remains to be done?
+* Include Markdown doc updates for user visible features
+* Spelling and grammar should be correct
+* Try to make formatting look as good as possible (use preview mode to check)
+* Follow [content](https://github.com/tektoncd/website/blob/master/content/en/doc-con-content.md)
+  and [formatting](https://github.com/tektoncd/website/blob/master/content/en/doc-con-formatting.md) guidelines
+* Should explain thoroughly how the new feature works
+* If possible, in addition to code snippets, include a reference to an end to end example
+* Ensure that all links and references are valid
 
-## Coding standards
+## Functionality
 
-### Go
+* It should be safe to cut a release at any time, i.e. merging this PR should not
+  put us into an unreleasable state
+    * When incrementally adding new features, this may mean that a release could contain
+      a partial feature, i.e. the type specification only but no functionality
+    * When introducting a partial feature, the documentation should include updates that
+      indicates clearly that this functionality is not expected to work and point the reader
+      toward how to follow progress (e.g. via an issue)
 
-- [Go code review comments](https://github.com/golang/go/wiki/CodeReviewComments)
+## Content
+
+* Whenever logic is added that uses a container image that wasn’t used before, the image used should
+  be configurable on the command line so that distributors can build images that meet their
+  support and licensing requirements
+* Refactoring should be merged separately from bug fixes and features
+  * i.e. if you refactor as part of implementing something, commit it and merge it before merging the change
+* Prefer small pull requests; if you can think of a way to break up the pull request into multiple, do it
+
+## Code
+
+* Reviewers are expected to understand the changes well enough that they would feel confident
+  saying the understand what is changing and why:
+  * Read through all the code changes
+  * Read through linked issues and pull requests, including the discussions
+* Prefer small well factored packages with unit tests
+* Pass kubernetes and tekton client functions into functions that need them as params so
+  they can be easily mocked in unit tests
+* [Go Code Review comments](https://github.com/golang/go/wiki/CodeReviewComments)
+  * All public functions and attributes have docstrings
+  * Don’t panic
+  * Error strings are not capitalized
+  * Handle all errors ([gracefully](https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully))
+    * When returning errors, add more context with `fmt.Errorf` and `%v`
+  * Use meaningful package names (avoid util, helper, lib)
+  * Prefer short variable names
+
+### Tests
+
+* New features (and often/whenever possible bug fixes) have one or all of:
+  * Examples (i.e. yaml tests)
+  * End to end tests
+* When API changes are introduced (e.g. changes to  `_types.go` files) corresponding changes are made to:
+  * Validation + validation tests
+* Unit tests:
+  * Coverage should remain the same or increase
+  * Test exported functions only, in isolation:
+    * Each exported function should have tests
+    * Each test should only test one function at a time
+    * If you find yourself wanting to test an unexported function, consider whether
+      it would make sense to move the test into another package and export it
+* Test code
+  * When using cmp.Diff the argument order is always (want, got) and in the error message include (-want +got)
+    (and/or use a lib like [PrintWantGot](https://github.com/tektoncd/pipeline/blob/master/test/diff/print.go))
+  * Table driven tests: do not use the same test for success and fail cases if the logic is different
+    (e.g. do not have two sets of test logic, gated with `wantErr`)
+
+### Reconciler/Controller changes
+
+For projects that have [CRD controllers](https://kubernetes.io/docs/concepts/extend-kubernetes/api-extension/custom-resources/#custom-controllers)
+(also known as "reconcilers"):
+
+* Avoid adding functions to the reconciler struct
+* Avoid adding functions to the reconciler package
+* [Return an error if you want the change to be re-queued](https://github.com/knative/pkg/blob/master/injection/README.md#generated-reconciler-responsibilities),
+  otherwise [return an permanent error](https://github.com/knative/pkg/blob/5358179e7499b1a3a4581d9d3673f391240ec86d/controller/controller.go#L516-L521)
+* Be sparing with the number of "reconciler" level tests as these are a kind of integration test
+  (they pull in all components in the reconciler) and tend to be slow (on the order of seconds)