Skip to content

Latest commit

 

History

History
152 lines (124 loc) · 7.58 KB

standards.md

File metadata and controls

152 lines (124 loc) · 7.58 KB

Tekton Pipelines Contributor and Reviewer Expectations

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. API changes should be evaluated according to Tekton Design Principles.

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

See also the Tekton review process.

Pull request description

  • 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)

Commits

  • Use the body to explain what and why vs. how. Link to an issue whenever possible and aim for 2 paragraphs, 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. 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).
    • Keep the subject to 50 characters or less.
    • Try to keep the message wrapped at 72 characters.
    • Check these seven best practices for more detail.

Example Commit Message

Here's a commit message example to work from that sticks to the spirit of the guidance outlined above:

Add example commit message to demo our guidance

Prior to this message being included in our standards there was no
canonical example of an "ideal" commit message for devs to quickly copy.

Providing a decent example helps clarify the intended outcome of our
commit message rules and offers a template for people to work from. We
could alternatively link to good commit messages in our repos but that
requires developers to follow more links rather than just showing
what we want.

Docs

  • 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 and formatting 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

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 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 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
    • All public functions and attributes have docstrings
    • Don’t panic
    • Error strings are not capitalized
    • Handle all errors (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)
    • 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 (also known as "reconcilers"):