You have made some changes to CockroachDB and you want to submit it to the CockroachDB team for inclusion in the main release.
How does this work? In summary, we will look at the following:
Is this your first time submitting a PR? Check Your first CockroachDB PR!
Every CockroachDB change that fixes a bug or changes functionality must be accompanied by testing code.
A unit test is good when:
it would fail if ran on CockroachDB before your change,
it succeeds when ran after the change,
it does not test too many other things besides the area you're changing,
it is fully automated.
If your change affects multiple areas, there should probably be a different unit test for each area (principle 3).
Currently in the CockroachDB project we use the following types of test:
Go unit tests, via functions named TestXXX placed in the same Go package as the change. This is the most common.
Use this when a change is local to a few functions and these functions can be exercised in isolation.
Take inspiration from the other tests in the same package.
Data-driven tests, ran via datadriven.RunTest, with test files in a testdata subdirectory.
If a package uses data-driven tests already, it's possible that you should use that too for your change.
SQL logic tests, ran via logictest.RunLogicTest, with test files in a testdata subdirectory.
If your change affects how CockroachDB runs SQL statements, you should probably include a SQL logic test.
Tests for CLI commands meant to be ran interactively in a terminal, using TCL, with test files in pkg/cli/interactive_tests.
If your change affects the cockroach command-line interface and only affects the behavior of interactive use, you may need this.
We also use non-interactive Example_xxxx tests in pkg/cli/cli_test.go.
Client language/framework tests in pkg/acceptance/adapter_test.go.
If your change adds feature to the client/server protocol, you probably need to try it out via this test infrastructure.
Vertical tests using roachtest in pkg/cmd/roachtest.
If testing your change require special flags in cockroach start, or if it needs to run for a longer time, you may want to use roachtest.
During a review, some attention is given to the coding style.
CockroachDB uses a couple extra formatting rules in addition to those used by gofmt. We provide the command crlfmt to enforce those automaticaly: Use:crlfmt -tab 2
We advise to integrate this in your text editor.
We value good naming for variables, functions for new concepts, etc. If your change extends an area where there is already an established naming pattern, you can reuse the existing pattern. If you are introducing new concepts, be creative! But during the review we might kindly request you to rename some of the entities.
We value in-line comments that clarify what is going on in the Go code.
Good comments:
are written in grammatical English, with full sentences (capital at beginning, period at end).
do not repeat what the code does (i.e. do not write // This loop iterates from 1 to 10 in front of a for-loop), but instead clarifies the goal and/or the motivation (e.g. // This loop aggregates a result over all the instances).
We value adequate (but not excessive) single empty lines to separate blocks of code that correspond to units of reasoning.
In some cases we even value adding comments on code that you have not changed, but that was unclear to you (when the comments would add a future reader).
Do not confuse the Git commit message and the PR description message!
As much as possible, put all the information about your change primarily in the git commit message, and merely maintain the PR description as a copy or a summary of the git commit message.
GitHub shows the PR description message, however, when developing, we get context and navigate code using the Git commit message.
The PR description message is auto-populated by GitHub from the Git commit message, so it "magically" works if the information is there in the first place.
If the discussion on a PR enables a new understanding, be careful to update the git commit message accordingly.
Place your release note(s) in git commit messages.
See the separate page on Git Commit Messages.
If your change affects the experience of many CockroachDB users (i.e. it affects a common case), we will validate that it fits with the mission to "make data easy".
If there is potential UX impact, we kindly request that you approach us to discuss this impact prior to starting to program the change. Depending on that discussion, we might express additional requirement that can influence the approach you'll take.
Short, simple projects produce a PR containing a single, small commit. However, it is often the case that a PR grows to contain multiple commits.
This happens mainly in two situations:
A larger project may be decomposed in sub-parts / steps. We recommend one commit per logical change in the PR, up to and including when the PR is merged. See below for details.
During code review, to answer reviewer comments. We accept seeing fix-up commits during the review, but we request they are merged together (“squashed”) before the PR is merged. See below for more nuance.
Generally, we do not wish to review PRs that contain more than a dozen commits. A large number of commits is a symptom that the change is disorganized and will put undue burden on the reviewer team. If you think you need to make a large change, please approach our team on Slack first to discuss how to organize the work.
See Organizing PRs and Commits for details.