Welcome, and thank you for considering contributing to Crossplane. We encourage you to help out by raising issues, improving documentation, fixing bugs, or adding new features
If you're interested in contributing please start by reading this document. If you have any questions at all, or don't know where to start, please reach out to us on Slack. Please also take a look at our code of conduct, which details how contributors are expected to conduct themselves as part of the Crossplane community.
The Crossplane project consists of several repositories under the crossplane and crossplane-contrib GitHub organisations. We're experimenting with Earthly in this repository (crossplane) and crossplane-runtime. Most other repositories use a
Makefile. To establish a development environment for a repository with aMakefile, try runningmake && make help.
Crossplane is written in Go. You don't need to have Go installed to contribute code to Crossplane but it helps to use an editor that understands Go.
To setup a Crossplane development environment:
Use the earthly command to build and test Crossplane. Run earthly doc to see
available build targets.
Useful targets include:
earthly +reviewable- Run code generators, linters, and unit tests.earthly -P +e2e- Run end-to-end tests.earthly +hack- Build Crossplane and deploy it to a localkindcluster.
Wondering whether something on the pull request checklist applies to your PR? Generally:
- Everyone must read and follow this contribution process.
- Every PR must run (and pass)
earthly +reviewable. - Most PRs that touch code should touch unit tests. We want ~80% coverage.
- Any significant feature should be covered by E2E tests. If you're adding a new feature, you should probably be adding or updating E2Es.
- Any significant feature should be documented. If you're adding a new feature, you should probably be opening a docs PR or tracking issue. If you make a change it's your responsibility to document it before it's released.
- Most PRs that (only) fix a bug should have a backport label.
If you're still unsure, just leave the checklist box unticked (and
un-struck-through). This will cause the checklist-completed CI job to fail
until you and your reviewer figure out what to do.
To contribute bug fixes or features to Crossplane:
- Communicate your intent.
- Make your changes.
- Test your changes.
- Update documentation and examples where appropriate.
- Open a Pull Request (PR).
Communicating your intent lets the Crossplane maintainers 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.
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.
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 section below and try to preempt any comments your
reviewers would otherwise leave. Run earthly +reviewable to lint your change.
All Crossplane features must be covered by unit and end-to-end (E2E) tests.
Crossplane uses table driven unit tests - you can find an example below. Crossplane does not use third-party test libraries (e.g. Ginkgo, Gomega, Testify) for unit tests and will request changes to any PR that introduces one. See the Go test review comments for our rationale.
E2E tests live under test/e2e. Refer to the E2E readme for information on
adding and updating E2E tests. They are considered to be expensive,
therefore add them only for important use cases that cannot be verified by
unit tests. If in a doubt, check with the maintainers for guidance.
All Crossplane documentation is under revision control; see the docs 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.
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 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.
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 unit and E2E tests for all changes.
- Preempt 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 if you have any questions about contributing!
By contributing to this project you agree to the Developer Certificate of Origin (DCO). This document was created by the Linux Kernel community and is a simple statement that you, as a contributor, have the legal right to make the contribution. See the DCO file for details.
Contributors sign-off that they adhere to these requirements by adding a Signed-off-by line to commit messages. For example:
This is my commit message
Signed-off-by: Random J Developer <[email protected]>
Git even has a -s command line option to append this automatically to your commit message:
git commit -s -m 'This is my commit message'If you have already made a commit and forgot to include the sign-off, you can amend your last commit to add the sign-off with the following command, which can then be force pushed.
git commit --amend -sWe use a DCO bot to enforce the DCO on all commits in every pull request.
All Pull Requests (PR), whether written by a Crossplane maintainer or a community member, must go through code review.
Not only do code reviews ensure that the code is correct, maintainable, and secure, but more importantly it allows us to use code reviews as an educational tool.
With this in mind, all efforts around code reviews should be seen through the lens of educating the author and be accompanied by kind, detailed feedback that will help authors understand the context the reviewer is coming from.
We encourage anyone in the community to conduct a code review on a PR. In most situations we prefer to have the following approvals before merging a PR:
- At least one approval from Reviewers
- At least one approval from Maintainers
When opening a PR, GitHub will assign reviewers based on the project's code review settings and who gets assigned depends on what code the contributor changed, per CODEOWNERS. In most cases we expect that someone from crossplane-reviewers and a subject matter expert from crossplane-maintainers will be assigned.
We encourage reviews from the community and Reviewers to take place before someone from the Maintainers group reviews a PR. This helps reduce the load on the project maintainers and ensures they can be more efficient in their reviews. In addition, reviewing PRs is a path to becoming a maintainer on the project.
While preparing your PR, be mindful of the instructions and requirements in the contributing code section.
Once your PR is ready for review please notify the assigned reviewers by mentioning them in a comment.
After implementing review feedback, the PR author should notify the reviewer by mentioning them in a comment when the PR is ready for another review.
If you are not getting a response within a reasonable timeframe, remembering
that reviewers and maintainers offer their time free of charge and have other
obligations, you can reach out to the #crossplane-owners channel in the
Crossplane community Slack workspace.
If you are assigned as a reviewer on a PR and you are unable to commit to reviewing the PR within a reasonable timeframe, you are encouraged to communicate this and manage the PR author's expectation.
All reviewers are encouraged to consider the following aspects and to provide guidance to the PR author before giving their approval:
- Is the code functionally correct?
- Are the changes well documented, and their intent explained sufficiently for current and future readers?
- Is the code written according to the Coding Style?
- Is the solution idiomatically aligned with existing Crossplane APIs?
- Is the code sufficiently covered by tests?
- Has the PR author signed the DCO?
- Are all CI jobs passing?
When providing feedback please consider the following guidelines:
- It helps the recipient a lot to be able to understand the context and intention behind comments.
- Aim to provide feedback in a conversational style, rather than terse instructions.
- Clearly articulate if you are sharing an opinion or instruction that needs to be complied with (i.e. a contribution guide rule). Proactively clarify what needs to change for you to feel comfortable to approve a PR.
- Default to asking questions when things are not how we would expect them to be. Suggesting rather than demanding changes.
- Proactively provide context when asking people to change things. Refer to where rules are defined or existing precedent exists, where possible.
- Allow the author to “win some battles”. Particularly if they’re pushing back on something that isn't crucial.
Examples:
- “What do you think about changing X? I think it would be an improvement because Y”.
- "Do we need X at all in this scenario? My thinking is: ..."
- "I like the direction this is going. I think adding X would be useful to Y.""
- "Please would you make sure your commits are signed (see the DCO check) and update the PR description per the template (in particular detail how you've tested this change)."
- "I'm not 100% sure I follow why this is needed - can you add a comment (to the code) explaining?"
- "I might be wrong, but I think what you're actually trying to do here is X"
Being specific with your intention and expectation can save hours of undercommunication and misunderstandings.
The Crossplane project 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.
These coding style guidelines apply to all https://github.com/crossplane and https://github.com/crossplane-contrib 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.
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. 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
you must:
- Be specific. Apply
//nolint:nameoflinterat as tight a scope as possible. - Include a comment explaining why you're disabling the linter.
For example:
func hash(s string) string {
h := fnv.New32()
_ = h.Write([]byte(s)) //nolint:errcheck // Writing to a hash never returns an error.
return fmt.Sprintf("%x", h.Sum32())
}Here we only disable the specific linter that would emit a warning (errcheck),
for the specific line where that warning would be emitted.
Quoting the Go code review comments:
Variable names in Go should be short rather than long. This is especially true for local variables with limited scope. Prefer
ctolineCount. PreferitosliceIndex.The basic rule: the further from its declaration that a name is used, the more descriptive the name must be. For a method receiver, one or two letters is sufficient. Common variables such as loop indices and readers can be a single letter (
i,r). More unusual things and global variables need more descriptive names.
Another way to frame the above is that we prefer to use short variables in all cases where a (human) reader could easily infer what the variable was from its source. For example:
// 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.
const NumberOfGeese = 42
// w is plenty for the first argument here. Naming it gooseWrangler is redundant
// because readers can tell what it is from its type. looseGeese on the other
// hand warrants a descriptive name. It's short lived (lines wise), and its type
// doesn't give us any context about what it's for.
func capture(w goose.Wrangler, looseGeese int) error {
// Important goose capturing logic.
for looseGeese > 0 {
// It's not obvious from the w.Wrangle method name what the
// return value is, so a descriptive name names sense here too.
captured, err := w.Wrangle()
if err != nil {
return errors.Wrap(err, "defeated by geese")
}
looseGeese = looseGeese - captured
}
// We prefer 'y' to 'yard' here because 'yard' is implied by 'NewYard'.
y := goose.NewYard(w)
return y.Secure()
}Quoting again from the Go code review comments:
Most of the time when people wrap lines "unnaturally" (in the middle of function calls or function declarations, more or less, say, though some exceptions are around), the wrapping would be unnecessary if they had a reasonable number of parameters and reasonably short variable names. Long lines seem to go with long names, and getting rid of the long names helps a lot.
func capture(gooseWrangler goose.Wrangler, looseGeese int, gooseYard goose.Yard,
duckWrangler duck.Wrangler, looseDucks, duckYard duck.Yard) error {
// Important fowl wrangling logic.
}If you find the need to wrap a function signature like the above it's almost always a sign that your argument names are superfluously verbose, or that your function is doing too much. If your function needs to take many optional arguments, perhaps to enable dependency injection, use variadic functions as options. In this case we usually make an exception for wrapped function calls. For example:
type Wrangler struct {
fw fowl.Wrangler
loose int
}
type Option func(w *Wrangler)
func WithFowlWrangler(fw fowl.Wrangler) Option {
return func(w *Wrangler) {
w.fw = fw
}
}
func NewWrangler(looseGeese int, o ...Option) *Wrangler {
w := &Wrangler{
fw: fowl.DefaultWrangler{}
loose:
}
for _, fn := range o {
fn(w)
}
return w
}
func example() {
w := NewWrangler(42,
WithFowlWrangler(chicken.NewWrangler()),
WithSomeOtherOption(),
WithYetAnotherOption())
w.Wrangle()
}You can read more about this pattern on Dave Cheney's blog.
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:
func example() error {
v := fetch()
if v == 42 {
// Really important business logic.
b := embiggen(v)
for k, v := range lookup(b) {
if v == true {
store(k)
} else {
remove(k)
}
}
return nil
}
return errors.New("v was a bad number")
}We prefer:
func example() error {
v := fetch()
if v != 42 {
return errors.New("v was a bad number")
}
// Really important business logic.
b := embiggen(v)
for k, v := range lookup(b) {
// "Continue early" is a variant of "return early".
if v == false {
remove(k)
continue
}
store(k)
}
return nil
}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).
Quoting Effective Go:
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.
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:
import "github.com/crossplane/crossplane-runtime/pkg/errors"
func example() error {
v, err := fetch()
if err != nil {
return errors.Wrap(err, "could not fetch the thing")
}
store(embiggen(v))
return nil
}Previously we made heavy use of error constants, for example:
const errFetch = "could not fetch the thing"
if err != nil {
return errors.Wrap(err, errFetch)
}We no longer recommend this pattern. Instead, you should mostly create or wrap errors with "inline" error strings. Refer to #4514 for context.
We recommend using cmpopts.EquateErrors to test that your code returns the
expected error. This cmp option will consider one error that errors.Is
another to be equal to it.
When testing a simple function with few error cases it's usually sufficient to
test simply whether or not an error was returned. You can use cmpopts.AnyError
for this. We prefer cmpopts.AnyError to a simple err == nil test because it
keeps our tests consistent. This way it's easy to mix and match tests that check
for cmpopts.AnyError with tests that check for a more specific error in the
same test table.
For example:
func TestQuack(t *testing.T) {
type want struct {
output string
err error
}
// We only care that Quack returns an error when supplied with a bad
// input, and returns no error when supplied with good input.
cases := map[string]struct{
input string
want want
}{
"BadInput": {
input: "Hello!",
want: want{
err: cmpopts.AnyError,
},
},
"GoodInput": {
input: "Quack!",
want: want{
output: "Quack!",
},
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got, err := Quack(tc.input)
if diff := cmp.Diff(got, tc.want.output); diff != "" {
t.Errorf("Quack(): -got, +want:\n%s", diff)
}
if diff := cmp.Diff(err, tc.want.err, cmpopts.EquateErrors()); diff != "" {
t.Errorf("Quack(): -got, +want:\n%s", diff)
}
})
}
}For more complex functions with many error cases (like Reconciler methods)
consider injecting dependencies that you can make return a specific sentinel
error. This way you're able to test that you got the error you'd expect given a
particular set of inputs and dependency behaviors, not another unexpected error.
For example:
func TestComplicatedQuacker(t *testing.T) {
// We'll inject this error and test we return an error that errors.Is
// (i.e. wraps) it.
errBoom := errors.New("boom")
type want struct {
output string
err error
}
cases := map[string]struct{
q Quacker
input string
want want
}{
"BadQuackModulator": {
q: &ComplicatedQuacker{
DuckIdentifer: func() (Duck, error) {
return &MockDuck{}, nil
},
QuackModulator: func() (int, error) {
// QuackModulator returns our sentinel
// error.
return 0, errBoom
}
},
input: "Hello!",
want: want{
// We want an error that errors.Is (i.e. wraps)
// our sentinel error. We don't test what error
// message it was wrapped with.
err: errBoom,
},
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got, err := tc.q.Quack(tc.input)
if diff := cmp.Diff(got, tc.want.output); diff != "" {
t.Errorf("q.Quack(): -got, +want:\n%s", diff)
}
if diff := cmp.Diff(err, tc.want.err, cmpopts.EquateErrors()); diff != "" {
t.Errorf("q.Quack(): -got, +want:\n%s", diff)
}
})
}
}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:
func example() error {
err := enable()
if err != nil {
return errors.Wrap(err, "could not enable the thing")
}
// 'err' still exists here at the function scope.
return errors.Wrap(emit(), "could not emit the thing")
}We prefer:
func example() error {
if err := enable(); err != nil {
// 'err' exists here inside the conditional block.
return errors.Wrap(err, "could not enable the thing")
}
// 'err' does not exist here at the function scope. It's scoped to the
// above conditional block.
return errors.Wrap(emit(), "could not emit the thing")
}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:
func example() error {
if v, err := fetch(); err != nil {
return errors.Wrap(err, "could not enable the thing")
} else {
store(embiggen(v))
}
return nil
}We prefer:
func example() error {
v, err := fetch()
if err != nil {
return errors.Wrap(err, "could not enable the thing")
}
store(embiggen(v))
return nil
}Conditions should be actionable for a user of Crossplane. This implies:
- conditions are made for users, not for developers.
- conditions should contain enough information to know where to look next.
- conditions are part of UX.
Conditions have a type, a reason and a message:
-
The type is fixed by type, e.g.
ReadyorSynced. Keep the number low. Uniform condition types across related kinds are preferred.Readyis common in Crossplane to indicate that a resource is ready to be used by the user. Do not signalReady=Trueearlier, e.g. do not signal a claim as ready before the credential secret has been created and has valid and working credentials. -
The reason is for machines and uses CamelCase. Reasons should be documented in the API docs.
-
The message is for humans and is written in plain English, without newlines, and with the first letter capitalized and no trailing punctuation. It might end in an error string, e.g.
Cannot create all resources: foo, bar, and 3 more failed: condiguration.pkg.crossplane.io "foo" is invalid: package.spec is required. Keep the message reasonable short although there is no hard limit. 1000 characters is probably too long, 100 characters is fine.
Conditions must not flap, including the reason and the message. Make sure that the reason and message are deterministic and stable. For example, sort in case of maps as maps iteration is not deterministic in Golang.
Avoid timestamps and in particular relative times in condition messages as these change on repeated reconciliation. Rule of thumb: if another reconcile shows the same problem, the condition message must not change.
Transient issues, e.g. apiserver conflict errors like the object has been modified; please apply your changes to the latest version and try again
must not be shown in condition messages, but rather the reconciliation should
silently requeue.
Events are for users, not for Crossplane developers. Events should matter for a human.
Events are about changes or actions. If nothing changes or no action happens, do not emit an event. For example, if no new composition is selected, do not emit an event. Successful idem-potent actions should only emit an event once. Erroring actions should emit an event for each error.
Events should aim at telling what has changed and to which value, e.g.
Successfully selected composition: eks.clusters.caas.com, don't omit the
composition name here.
Events should not be used to tell what is going to happen, but what has happened. In reconcile functions with an update at the end, it is fine to emit an event before the update, in the assumption that the update will succeed.
Transient issues, e.g. apiserver conflict errors like the object has been modified; please apply your changes to the latest version and try again
should not be emitted as an event, but rather the reconciliation should silently
requeue.
Events are not a replacements for conditions. As a rule of thumb: the last event showing a problem should show up as condition message too.
To keep the value for the user up, keep the number of events low. Events are for humans and humans will read 10, but not 1000 events per object. Emit events valuable for the user. Use logs instead of events for higher volume information.
Examples for good events:
Successfully selected composition: eks.clusters.caas.com– the message is stable, and this is an action (selecting) that succeeded. Hence, it is fine to emit one event for it.Readiness probe failed: Get "https://192.168.139.246:8443/readyz": net/http: request canceled (Client.Timeout exceeded while awaiting headers)– the error string is stable, and this is an actions (probing) that failed. Hence, it is fine to repeat the event.
Examples for bad events:
Applied RBAC ClusterRoles– it's lacking which ClusterRoles.Bound system ClusterRole to provider ServiceAccount(s)– it's lacking which ClusterRole, which service accounts and what this cluster role enables.(Re)started composite resource controller– controllers are not user-facing, but just an implementation detail of how APIs are implemented.Update failed: the object has been modified; please apply your changes to the latest version and try again– it's lacking which update failed. Moreover, this is a transient apiserver error. The controller should silently requeue instead of emitting an event.
As mentioned in 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:
// Example is the function we're testing.
func Example(ctx context.Context, input string) (int, error) {
// ...
}
// Test function names are always PascalCase. No underscores.
func TestExample(t *testing.T) {
type args struct {
ctx context.Context
input string
}
type want struct {
output int
err error
}
cases := map[string]struct{
reason string
args args
want want
}{
// The summary is always PascalCase. No spaces, hyphens, or underscores.
"BriefTestCaseSummary": {
reason: "A longer summary of what we're testing - printed if the test fails.",
args: args{
ctx: context.Background(),
input: "some input value",
}
want: want{
output: "the expected output",
err: nil,
}
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got, err := Example(tc.args.ctx, tc.args.input)
// 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.
if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" {
t.Errorf("%s\nExample(...): -want, +got:\n%s", tc.reason, diff)
}
if diff := cmp.Diff(tc.want.output, got); diff != "" {
t.Errorf("%s\nExample(...): -want, +got:\n%s", tc.reason, diff)
}
})
}
}