Skip to content

Commit

Permalink
[Code standards] Add detail on small PRs and Go packages
Browse files Browse the repository at this point in the history 
This commit updates code standards to include guidance on pull request size,
structuring Go packages, and better release notes.

It also removes language around being in a "releasable state".
It's difficult to write a PR that puts us in an unreleasable state,
since our CI prevents this, so guidance that PRs shouldn't put us in
an unreleasable state adds confusion.
  • Loading branch information
@lbernick
lbernick committed Jan 13, 2023
1 parent 72959f6 commit 35ff9c0
Showing 1 changed file with 55 additions and 32 deletions.
87 changes: 55 additions & 32 deletions standards.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,32 @@ Design: Most designs should be first proposed via an issue and possibly a
API changes should be evaluated according to
[Tekton Design Principles](https://github.com/tektoncd/community/blob/main/design-principles.md).

Pull request reviewers are expected to meet [reviewer responsibilities](#reviewer-responsibilities).

Each Pull Request is expected to meet the following expectations around:

* [Pull Request Description](#pull-request-description)
* [Release Notes](#release-notes)
* [Commits](#commits)
* [Commit Messages](#commits)
* [Example Commit Message](#example-commit-message)
* [Small Pull Requests](#small-pull-requests)
* [Docs](#docs)
* [Functionality](#functionality)
* [Content](#content)
* [Code](#code)
* [Go packages](#go-packages)
* [Tests](#tests)
* [Reconciler/Controller Changes](#reconcilercontroller-changes)

_See also [the Tekton review process](https://github.com/tektoncd/community/blob/main/process.md#reviews)._

## Reviewer Responsibilities

* Reviewers are expected to understand the changes well enough that they would feel confident
saying they understand what is changing and why:
* Read through all the code changes
* Read through linked issues and pull requests, including the discussions

## Pull request description

* Include a link to the issue being addressed, but describe the context for the reviewer
Expand Down Expand Up @@ -55,7 +66,7 @@ Refer to the following set of questions to help fill the release-note section.
* If this PR addresses a publicly known CVE, include the CVE number in the release notes
* If unsure, include release note.
* It's recommended to include release note explaining the changes in the PR.

* If a feature is implemented incrementally, use a release note only when it is complete and ready to be used

### None Release Note

Expand All @@ -69,7 +80,7 @@ NONE
```
````

### Example Release Note
### Example Release Notes

#### Poor Release Note

Expand All @@ -92,7 +103,7 @@ Workspaces are propagated in embedded specifications without mutations.
```
````

A few examples of good release-notes:
#### Good Release Notes


* Reasonable release note for introducing a new feature:
Expand All @@ -112,7 +123,17 @@ Refer to the TEP-00XX for more details.
```
````

## Commits
While commit messages explain what changes are being made to the code and why, good release notes should explain the impact on the user.
For example, if your commit message title is "Add resolvers deployment, with release and e2e integration",
a great release note could be:

````
```release-note
action required: The separate Resolutions project has been folded into Pipeline. If currently using Resolution, remove the tekton-remote-resolution namespace before upgrading and installing the new "resolvers.yaml".
```
````

## Commit Messages

* Use the body to explain [what and why vs. how](https://chris.beams.io/posts/git-commit/#why-not-how).
Link to an issue whenever possible and [aim for 2 paragraphs](https://www.youtube.com/watch?v=PJjmw9TRB7s),
Expand All @@ -122,7 +143,6 @@ Refer to the TEP-00XX for more details.
* 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. For multiple commits ensure each makes sense without the context of the others.
* As much as possible try to stick to these general formatting guidelines:
* Separate subject line from message body.
* Write the subject line using the "imperative mood" ([see examples](https://chris.beams.io/posts/git-commit/#imperative)).
Expand All @@ -149,6 +169,20 @@ requires developers to follow more links rather than just showing
what we want.
```

## Small Pull Requests

* Small pull requests that make a single, self-contained change are easier to review and easier to roll back or backport.
* If you can think of a way to break up a pull request into smaller changes, do it
* Tests and documentation (where applicable) should always be part of the same pull request when introducing a change
* Use one commit per PR unless there is a strong reason not to
* For multiple commits, ensure each makes sense without the context of the others.
* Refactoring should be merged before bug fixes and features
* i.e. if you refactor as part of implementing something, commit it and merge it before merging the change
* Features may be implemented incrementally to keep pull requests small
* When introducing a partial feature, the documentation should include updates that
indicate clearly that this functionality is not expected to work and point the reader
toward how to follow progress (e.g. via an issue)

## Docs

* Include Markdown doc updates for user visible features
Expand All @@ -160,33 +194,15 @@ what we want.
* 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

* 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 introducing a partial feature, the documentation should include updates that
indicate 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 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

* Tekton projects follow the [Go Style Guide](https://google.github.io/styleguide/go/).
* 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
* Tekton projects follow the [Go Style Guide](https://google.github.io/styleguide/go/) and [Effective Go](https://go.dev/doc/effective_go).
* 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)
Expand All @@ -195,23 +211,30 @@ what we want.
* 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 packages

* Prefer small well factored packages with unit tests
* Use meaningful package names (avoid util, helper, lib). See https://go.dev/blog/package-names
* Only export functions that provide a meaningful API for consumers of the package
* All exported functions should have tests
* Test exported functions only, since these are the functions package consumers will use.
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,
or to refactor the package so that tests use the same functions as package importers
* If your package is named "foo", prefer putting tests in a "foo_test" package to ensure that only exported functions are tested

### 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:
* 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
* Each test should only test one function at a time
* 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/main/test/diff/print.go))
Expand Down

0 comments on commit 35ff9c0

Please sign in to comment.