Skip to content

Commit

Permalink
CONTRIBUTING: Adapt Crossplane guidance to SIG Release
Browse files Browse the repository at this point in the history
Signed-off-by: Stephen Augustus <[email protected]>
  • Loading branch information
justaugustus committed Feb 26, 2022
1 parent c88f00d commit de4dca8
Showing 1 changed file with 111 additions and 83 deletions.
194 changes: 111 additions & 83 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ _As contributors and maintainers of this project, and in the interest of fosteri

- [Getting Started](#getting-started)
- [Mentorship](#mentorship)
- [Contact Information](#contact-information)
- [Contributing Code](#contributing-code)
- [Coding Style](#coding-style)
- [Explain 'nolint' Directives](#explain-nolint-directives)
Expand Down Expand Up @@ -34,15 +35,12 @@ If your repo has certain guidelines for contribution, put them here ahead of the

- [Mentoring Initiatives](https://git.k8s.io/community/mentoring) - We have a diverse set of mentorship programs available that are always looking for volunteers!

<!---
Custom Information - if you're copying this template for the first time you can add custom content here, for example:
## Contact Information

- [Slack channel](https://kubernetes.slack.com/messages/kubernetes-users) - Replace `kubernetes-users` with your slack channel string, this will send users directly to your channel.
- [Mailing list](URL)
<!-- TODO(contact): Dedupe with README and k/community -->

-->
- [Slack channel](https://kubernetes.slack.com/messages/sig-release)
- [Mailing list](https://groups.google.com/forum/#!forum/kubernetes-sig-release)

## Contributing Code

Expand All @@ -54,93 +52,111 @@ To contribute bug fixes or features to Crossplane:
1. Update documentation and examples where appropriate.
1. Open a Pull Request (PR).

Communicating your intent lets the Crossplane maintainers know that you intend
Communicating your intent lets SIG Release members know that you intend
to contribute, and how. This sets you up for success - you can avoid duplicating
an effort that may already be underway, adding a feature that may be rejected,
or heading down a path that you would be steered away from at review time. The
best way to communicate your intent is via a detailed GitHub issue. Take a look
first to see if there's already an issue relating to the thing you'd like to
contribute. If there isn't, please raise a new one! Let us know what you'd like
to work on, and why. The Crossplane maintainers can't always triage new issues
immediately, but we encourage you to bring them to our attention via [Slack].
to work on, and why. SIG Release members can't always triage new issues
immediately, but we encourage you to bring them to our attention via one of our
[communication channels][contact].

> NOTE: Unlike the core [Kubernetes release cycle][kubernetes-release-cycle],
> [SIG Release repositories][sig-release-subprojects] are versioned/released on
> ad hoc (as needed basis), which means that the changes you've made will
> not necessarily be immediately available for consumption in tagged artifacts.
>
> We may release components for the following, non-exhaustive reasons:
>
> - unblocking a core Kubernetes release cycle
> - unblocking Kubernetes projects that leverage our libraries
> - mitigating a security vulnerability
> - critical bug fixes or regressions
> - a feature is incomplete or does not work as intended
>
> If you need a new component release to unblock your project, please contact us to request one.
> NOTE: new features can only being merged during the active development period
> of a Crossplane release cycle. If implementation and review of a new feature
> cannot be accomplished prior to feature freeze, it may be bumped to the next
> release cycle. See the [Crossplane release cycle] documentation for more
> information.
<!--
TODO(contribute): Review Crossplane release process for more inspiration
ref: https://github.com/crossplane/crossplane/blob/master/docs/reference/release-cycle.md
-->

Be sure to practice [good git commit hygiene] as you make your changes. All but
the smallest changes should be broken up into a few commits that tell a story.
Use your git commits to provide context for the folks who will review PR, and
the folks who will be spelunking the codebase in the months and years to come.
Ensure each of your commits is signed-off in compliance with the [Developer
Certificate of Origin] by using `git commit -s`. The Crossplane project highly
values readable, idiomatic Go code. Familiarise yourself with the
[Coding Style](#coding-style) section below and try to preempt any comments your
reviewers would otherwise leave. Run `make reviewable` to lint your change.

All Crossplane code must be covered by tests. Note that unlike many Kubernetes
projects Crossplane does not use Ginkgo tests and will request changes to any PR
that uses Ginkgo or any third party testing library, per the common Go [test
review comments]. Crossplane encourages the use of table driven unit tests - you
can find an example [below](#prefer-table-driven-tests). Note that when opening
a PR your reviewer will expect you to detail how you've tested your work. For
all but the smallest changes some manual testing is encouraged in addition to
unit tests.

All Crossplane documentation is under revision control; see the [docs] directory
of this repository. Any change that introduces new behaviour or changes existing
behaviour must include updates to any relevant documentation. Please keep
documentation changes in distinct commits.
Ensure each of your commits is signed-off by using `git commit -s`. SIG Release
members highly values readable, idiomatic Go code. Familiarize yourself with
the [Coding Style](#coding-style) section below and try to preempt any comments
your reviewers would otherwise leave. Most repos will contain `Makefiles`s or
[`mage`][mage] targets that will help you build/test/lint/verify your changes.
If you don't see a target for your use case, maybe that could be your first
contribution!

<!--
TODO(contribute): Decide on stance around testing libraries: https://github.com/golang/go/wiki/TestComments#assert-libraries
-->

All code should be covered by tests. We strive, wherever possible, to follow
the common Go [test review comments]. We encourage the use of table-driven
unit tests - you can find an example [below](#prefer-table-driven-tests). Note
that when opening a PR your reviewer will expect you to detail how you've
tested your work. For all but the smallest changes, some manual testing is
encouraged in addition to unit tests.

All documentation is under revision control. Any change that introduces new
behavior or changes existing behavior must include updates to any relevant
documentation. Please keep documentation changes in distinct commits.

Once your change is written, tested, and documented the final step is to have it
reviewed! You'll be presented with a template and a small checklist when you
open a PR. Please read the template and fill out the checklist. Please make all
requested changes in subsequent commits. This allows your reviewers to see what
has changed as you address their comments. Be mindful of your commit history as
has changed as you address their comments. Be mindful of your commit history as
you do this - avoid commit messages like "Address review feedback" if possible.
If doing so is difficult a good alternative is to rewrite your commit history to
clean them up after your PR is approved but before it is merged.
If doing so is difficult a good alternative is to rewrite your commit history
to clean them up after your PR is approved but before it is merged.

In summary, please:

* Discuss your change in a GitHub issue before you start.
* Use your Git commit messages to communicate your intent to your reviewers.
* Sign-off on all Git commits by running `git commit -s`
* Add or update tests for all changes.
* Preempt [coding style](#coding-style) review comments.
* Update all relevant documentation.
* Don't force push to address review feedback. Your commits should tell a story.
* If necessary, tidy up your git commit history once your PR is approved.
- Discuss your change in a GitHub issue before you start.
- Use your Git commit messages to communicate your intent to your reviewers.
- Sign-off on all git commits by running `git commit -s`
- Add or update tests for all changes.
- Preempt [coding style](#coding-style) review comments.
- Update all relevant documentation.
- Don't force push to address review feedback. Your commits should tell a story.
- If necessary, tidy up your git commit history once your PR is approved.

Thank you for reading through our contributing guide! We appreciate you taking
the time to ensure your contributions are high quality and easy for our
community to review and accept. Please don't hesitate to [reach out to
us][Slack] if you have any questions about contributing!
us][contact] if you have any questions about contributing!

## Coding Style

The Crossplane project prefers not to maintain its own style guide, but we do
SIG Release prefers not to maintain its own style guide, but we do
enforce the style and best practices established by the Go project and its
community. This means contributors should:

* Follow the guidelines set out by the [Effective Go] document.
* Preempt common Go [code review comments] and [test review comments].
* Follow Crossplane's [Observability Developer Guide].
- Follow the guidelines set out by the [Effective Go] document.
- Preempt common Go [code review comments] and [test review comments].

These coding style guidelines apply to all https://github.com/crossplane and
https://github.com/crossplane-contrib repositories unless stated otherwise.
These coding style guidelines apply to all SIG Release repositories unless
stated otherwise.

Below we cover some of the feedback we most frequently leave on pull requests.
Most of these are covered by the documents above, but may be subtle or easily
missed and thus warrant closer attention.

### Explain 'nolint' Directives

We use [golangci-lint] on all our repositories to enforce many style and safety
rules that are not covered here. We prefer to tolerate false positives from our
linter configuration in order to make sure we catch as many issues as possible.
We use [golangci-lint] on all our Golang-based repositories to enforce many
style and safety rules that are not covered here. We prefer to tolerate false
positives from our linter configuration in order to make sure we catch as many
issues as possible.
This means it's sometimes necessary to override the linter to make a build pass.

You can override the linter using a `//nolint` comment directive. When you do so
Expand Down Expand Up @@ -181,7 +197,6 @@ cases where a (human) reader could easily infer what the variable was from its
source. For example:

```go

// NumberOfGeese might be used outside this package, or many many lines further
// down the file so it needs a descriptive name. It's also just an int, which
// doesn't give the reader much clue about what it's for.
Expand Down Expand Up @@ -276,7 +291,7 @@ You can read more about this pattern on [Dave Cheney's blog].
### Return Early

We prefer to return early. Another way to think about this is that we prefer to
handle terminal cases (e.g. errors) early. So for example instead of:
handle terminal cases (e.g., errors) early. So for example instead of:

```go
func example() error {
Expand Down Expand Up @@ -321,21 +336,27 @@ func example() error {

This approach gets error handling out of the way first, allowing the 'core' of
the function to follow at the scope of the function, not a conditional. Or put
otherwise, with the least amount of indentation. An interesting side effect of
this approach is that it's rare to find an `else` in Crossplane code (at the
time of writing there are four uses of `else` in `crossplane/crossplane`).
otherwise, with the least amount of indentation.

Quoting [Effective Go]:

> In the Go libraries, you'll find that when an if statement doesn't flow into
> In the Go libraries, you'll find that when an `if` statement doesn't flow into
> the next statement—that is, the body ends in break, continue, goto, or
> return—the unnecessary else is omitted.
### Wrap Errors

Use [`crossplane-runtime/pkg/errors`] to wrap errors with context. This allows
us to emit logs and events with useful, specific errors that can be related to
deeper parts of the codebase without having to actually plumb loggers and event
sources deep down into the codebase. For example:
<!--
TODO(contribute): Consider error packages like
[`crossplane-runtime/pkg/errors`]
-->

Always wrap errors with context. This allows us to emit logs and events with
useful, specific errors that can be related to deeper parts of the codebase
without having to actually plumb loggers and event sources deep down into the
codebase.

For example:

```go
import "github.com/crossplane/crossplane-runtime/pkg/errors"
Expand All @@ -356,8 +377,9 @@ func example() error {
Where possible, keep errors as narrowly scoped as possible. This avoids bugs
that can appear due to 'shadowed' errors, i.e. accidental re-use of an existing
`err` variable, as code is refactored over time. Keeping errors scoped to the
error handling conditional block can help protect against this. So for example
instead of:
error handling conditional block can help protect against this.

So for example, instead of:

```go
func example() error {
Expand Down Expand Up @@ -390,7 +412,9 @@ func example() error {

Note that the 'return early' advice above trumps this rule - it's okay to
declare errors at the function scope if it lets you keep business logic less
nested. That is, instead of:
nested.

That is, instead of:

```go
func example() error {
Expand Down Expand Up @@ -420,11 +444,11 @@ func example() error {

### Prefer Table Driven Tests

As mentioned in [Contributing Code](#contributing-code) Crossplane diverges from
common controller-runtime patterns in that it follows the advice laid out in the
Go project's [test review comments] documents. This means we prefer table driven
tests, and avoid test frameworks like Ginkgo. The most common form of Crossplane
test is as follows:
As mentioned in [Contributing Code](#contributing-code), we defer to the advice
laid out in the Go project's [test review comments] documents. This means we
prefer table-driven tests.

The most common form of a test is as follows:

```go
// Example is the function we're testing.
Expand Down Expand Up @@ -469,8 +493,7 @@ func TestExample(t *testing.T) {

// We prefer to use https://github.com/google/go-cmp/
// even for simple comparisons to keep test output
// consistent. Some Crossplane specific cmp options can
// be found in crossplane-runtime/pkg/test.
// consistent.
if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
t.Errorf("%s\nExample(...): -want, +got:\n%s", tc.reason, diff)
}
Expand All @@ -485,6 +508,10 @@ func TestExample(t *testing.T) {

## Establishing a Development Environment

<!--
TODO(contribute): Evaluate Crossplane guidance on establishing a development
environment.
The Crossplane project consists of several repositories under the crossplane and
crossplane-contrib GitHub organisations. Most of these projects use the Upbound
[build submodule]; a library of common Makefiles. Establishing a development
Expand All @@ -497,9 +524,9 @@ environment typically requires:
Run `make help` for information on the available Make targets. Useful targets
include:
* `make reviewable` - Run code generation, linters, and unit tests.
* `make e2e` - Run end-to-end tests.
* `make` - Build Crossplane.
- `make reviewable` - Run code generation, linters, and unit tests.
- `make e2e` - Run end-to-end tests.
- `make` - Build Crossplane.
Once you've built Crossplane you can deploy it to a Kubernetes cluster of your
choice. [`kind`] (Kubernetes in Docker) is a good choice for development. The
Expand Down Expand Up @@ -535,6 +562,9 @@ kubectl -n crossplane-system scale deploy crossplane --replicas=0
# is your active kubectl context. You can also go run cmd/crossplane/main.go.
make run
```
-->

Coming soon!

## Attribution

Expand All @@ -558,23 +588,21 @@ the following Crossplane contributors:
- [nchetan](https://github.com/nchetan)
- [muvaf](https://github.com/muvaf)

[Slack]: https://slack.crossplane.io/
[code of conduct]: https://github.com/cncf/foundation/blob/master/code-of-conduct.md
[contact]: #contact-information
[crossplane]: https://github.com/crossplane
[crossplane-contributing]: https://github.com/crossplane/crossplane/blob/master/CONTRIBUTING.md
[crossplane-contributing-commit]: https://github.com/crossplane/crossplane/blob/2c10ff0e81b83d609cfa81c3a24ce842ab234c38/CONTRIBUTING.md
[build submodule]: https://github.com/upbound/build/
[`kind`]: https://kind.sigs.k8s.io/
[Crossplane release cycle]: docs/reference/release-cycle.md
[kubernetes-release-cycle]: https://kubernetes.io/releases/release/
[good git commit hygiene]: https://www.futurelearn.com/info/blog/telling-stories-with-your-git-history
[Developer Certificate of Origin]: https://github.com/apps/dco
[code review comments]: https://github.com/golang/go/wiki/CodeReviewComments
[test review comments]: https://github.com/golang/go/wiki/TestComments
[crossplane-runtime]: https://github.com/crossplane/crossplane-runtime
[docs]: docs/
[Effective Go]: https://golang.org/doc/effective_go
[Observability Developer Guide]: docs/contributing/observability_developer_guide.md
[Dave Cheney's blog]: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis
[`crossplane-runtime/pkg/errors`]: https://pkg.go.dev/github.com/crossplane/crossplane-runtime/pkg/errors
[golangci-lint]: https://golangci-lint.run/
[mage]: https://magefile.org/
[sig-release-subprojects]: https://git.k8s.io/community/sig-release#subprojects
[template-project]: https://git.k8s.io/kubernetes-template-project

0 comments on commit de4dca8

Please sign in to comment.