From 9db2b4ad32b1b0735c0eaca2b922fcbdea1b0bf7 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Fri, 19 Jun 2020 15:47:03 -0400 Subject: [PATCH 1/2] Add contributor and reviewer expectations These started out as just being for Tekton Pipelines, but @dibyom pointed out they make sense for triggers too, so let's see if we want to adopt these for all the tektoncd projects or if we just want to constrain it to triggers + pipelines. The goal of these guidelines is to make sure that contributors understand what is expected in their submissions and also so that a reviewer (especially if someone wants to join and start reviewing who hasn't reviewed as part of this project before) will know when they approve + lgtm what they are expected to have checked. --- standards.md | 143 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 111 insertions(+), 32 deletions(-) diff --git a/standards.md b/standards.md index dc71ecacf..e93910db5 100644 --- a/standards.md +++ b/standards.md @@ -1,47 +1,126 @@ -# 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: +Each Pull Request is expected to meet the following expectations around: -- **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 +* [Pull Request Description](#pull-request-description) +* [Commits](#commits) +* [Docs](#docs) +* [Tests](#tests) +* [API Design](#api-design) +* [Functionality](#functionality) +* [Content](#content) +* [Code](#code) -Minimize the number of integration tests written and maximize the unit tests! -Unit test coverage should increase or stay the same with every PR. +_See also [the Tekton review process](https://github.com/tektoncd/community/blob/master/process.md#reviews)._ -This means that most PRs should include both: +## Pull request description -1. Tests -2. Documentation updates +* 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 + * Release notes filled in for user visible changes (bugs + features), + or removed if not applicable (refactoring, updating tests) + * If the template contains a checklist, it should be checked off -## Commit Messages +## Commits -All commit messages should follow -[these best practices](https://chris.beams.io/posts/git-commit/), specifically: +* 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 -- 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/)) +## Docs -Aim for [2 paragraphs in the body](https://www.youtube.com/watch?v=PJjmw9TRB7s). -Not sure what to put? Include: +* 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 -- 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? +## Tests -## Coding standards +* New features (and often/whenever possible bug fixes) have one or both of: + * Examples (i.e. yaml tests) + * End to end tests + * Reconciler tests +* New types have: + * 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`) -### Go +## API Design + +* Avoid kubernetes specific feature in our APIs as much as possible +* If we don’t definitely need the feature, don’t add it + * Must be backed up with [CI/CD specific use cases](https://github.com/tektoncd/community/blob/master/roadmap.md#mission-and-vision) +* Prefer keeping the API simple: + * If you can avoid adding a new feature, don’t add it + * Prefer reusing existing features to adding new ones + +## Functionality + +* 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 + +## Content + +* Whenever logic is added that uses an 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 +* Reconciler changes: + * 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, otherwise do not + (better yet, use [permanent errors](https://github.com/tektoncd/pipeline/issues/2474)) +* 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 -- [Go code review comments](https://github.com/golang/go/wiki/CodeReviewComments) \ No newline at end of file From e4897c55e05ae27721907315668a1115256df8ea Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Tue, 3 Nov 2020 16:06:27 -0500 Subject: [PATCH 2/2] =?UTF-8?q?Update=20standards=20with=20the=20latest=20?= =?UTF-8?q?and=20greatest!=20=F0=9F=8F=86?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since first creating this PR we've had some really useful improvements that I've tried to encorporate: * The TEP process! * Design principles now have their own doc at https://github.com/tektoncd/community/blob/master/design-principles.md#conformance * release-notes plugin automation via Prow * Permanent errors support was merged Also made improvements from feedback such as: * Putting the reconciler/controller specific recommendations into their own block * Requiring links and references in docs be validated (thanks @popcor255 !) --- standards.md | 88 ++++++++++++++++++++++++++++------------------------ 1 file changed, 48 insertions(+), 40 deletions(-) diff --git a/standards.md b/standards.md index e93910db5..dbbe6540f 100644 --- a/standards.md +++ b/standards.md @@ -5,16 +5,21 @@ The purpose of this doc is to: * 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 +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). + Each Pull Request is expected to meet the following expectations around: * [Pull Request Description](#pull-request-description) * [Commits](#commits) * [Docs](#docs) -* [Tests](#tests) -* [API Design](#api-design) * [Functionality](#functionality) * [Content](#content) * [Code](#code) + * [Tests](#tests) + * [Reconciler/Controller Changes](#reconcilercontroller-changes) _See also [the Tekton review process](https://github.com/tektoncd/community/blob/master/process.md#reviews)._ @@ -24,9 +29,10 @@ _See also [the Tekton review process](https://github.com/tektoncd/community/blob * 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 - * Release notes filled in for user visible changes (bugs + features), - or removed if not applicable (refactoring, updating tests) * 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)) ## Commits @@ -55,36 +61,8 @@ _See also [the Tekton review process](https://github.com/tektoncd/community/blob * 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 - -## Tests - -* New features (and often/whenever possible bug fixes) have one or both of: - * Examples (i.e. yaml tests) - * End to end tests - * Reconciler tests -* New types have: - * 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`) - -## API Design - -* Avoid kubernetes specific feature in our APIs as much as possible -* If we don’t definitely need the feature, don’t add it - * Must be backed up with [CI/CD specific use cases](https://github.com/tektoncd/community/blob/master/roadmap.md#mission-and-vision) -* Prefer keeping the API simple: - * If you can avoid adding a new feature, don’t add it - * Prefer reusing existing features to adding new ones +* If possible, in addition to code snippets, include a reference to an end to end example +* Ensure that all links and references are valid ## Functionality @@ -92,10 +70,13 @@ _See also [the Tekton review process](https://github.com/tektoncd/community/blob 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) ## Content -* Whenever logic is added that uses an image that wasn’t used before, the image used should +* 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 @@ -103,15 +84,11 @@ _See also [the Tekton review process](https://github.com/tektoncd/community/blob * 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 -* Reconciler changes: - * 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, otherwise do not - (better yet, use [permanent errors](https://github.com/tektoncd/pipeline/issues/2474)) * 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 @@ -124,3 +101,34 @@ _See also [the Tekton review process](https://github.com/tektoncd/community/blob * 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)