Permalink
May 23, 2017
May 23, 2017
May 23, 2017
May 23, 2017
Aug 8, 2017
May 23, 2017
May 23, 2017
May 23, 2017
May 23, 2017
May 23, 2017
May 23, 2017
Apr 24, 2018
May 23, 2017
May 29, 2017
May 23, 2017
Oct 15, 2017
May 23, 2017
May 23, 2017
Mar 28, 2018
May 23, 2017
May 23, 2017
May 23, 2017
May 29, 2017
May 23, 2017
May 23, 2017
Oct 15, 2017
Newer
100644
640 lines (497 sloc)
29.8 KB
1
# Submitting your first PR
2
3
*Original author: Nikhil Benesch <<benesch@cockroachlabs.com>>*
4
5
This document is a long-winded guide to preparing and submitting your first
6
contribution to CockroachDB. It's primarily intended as required reading for new
7
Cockroach Labs engineers, but may prove useful to external contributors too.
8
9
**Table of contents**
10
+ [The development cycle](#the-development-cycle)
11
+ [First things first](#first-things-first)
12
+ [Repository layout](#repository-layout)
13
+ [Other important repositories](#other-important-repositories)
14
+ [Internal documentation](#internal-documentation)
15
+ [Start coding!](#start-coding)
16
+ [Is there a Go debugger?](#is-there-a-go-debugger)
17
+ [A note on adding a new file](#a-note-on-adding-a-new-file)
18
+ [When to submit](#when-to-submit)
19
+ [Production-ready code](#production-ready-code)
20
+ [How to submit](#how-to-submit)
21
+ [Fix your style violations](#fix-your-style-violations)
22
+ [Where to push](#where-to-push)
23
+ [Split your change into logical chunks](#split-your-change-into-logical-chunks)
24
+ [Who to request a review from](#who-to-request-a-review-from)
25
+ [Write your PR description](#write-your-pr-description)
26
+ [TeamCity](#teamcity)
27
+ [The impending code review](#the-impending-code-review)
28
+ [Reviewable](#reviewable)
29
+ [Addressing feedback](#addressing-feedback)
30
+ [CR Jargon](#cr-jargon)
31
+ [Merging](#merging)
32
+ [First PR checklist](#first-pr-checklist)
33
34
## The development cycle
35
36
At the time of this writing, CockroachDB is several hundred thousand lines of Go
37
code, plus a smattering of C++ and TypeScript. Especially if you've never worked
38
with Go, this can be daunting. This section is a whirlwind tour of what lives
39
where, but you shouldn't expect all this information to permanently sink in
40
until well after your first month.
41
42
### First things first
43
44
Since CockroachDB is open source, most of the instructions for hacking on
45
CockroachDB live in this repo, [cockroachdb/cockroach]. You should start by
46
reading [CONTRIBUTING.md] in full. This file covers how to set up a new
47
development machine to build CockroachDB from source and the basics of the code
48
review workflow.
49
51
Review](https://code.google.com/p/go-wiki/wiki/CodeReviewComments) guide it
52
links to. These are linked to from [CONTRIBUTING.md], but they're easy to miss.
53
If you haven't written any Go before CockroachDB, you may want to hold off on
54
reviewing the style guide until you've written your first few functions in go.
55
56
### Repository layout
57
58
Here's what's in each top-level directory in this repository:
59
61
Build support scripts. You'll likely only need to
62
interact with [build/builder.sh]. See ["Building on Linux"] below.
63
65
Glue to convince our build system to build non-Go dependencies. At the time
66
of writing, "non-Go dependencies" means C or C++ dependencies. The most
67
important of these is [RocksDB], our underlying persistent key-value store.
68
69
+ **`cloud/kubernetes/`**
70
Kubernetes configuration to auto-launch CockroachDB clusters.
71
73
Documentation for CockroachDB developers. See ["Internal
74
documentation"] below.
75
77
Configuration to integrate monitoring frameworks, namely Prometheus and
78
Grafana, with CockroachDB. This configuration powers our internal monitoring
79
dashboard as well.
80
82
First-party Go code. See ["Internal documentation"] below for details.
83
85
Handy shell scripts that aren't part of the build process. You'll likely
86
interact with [scripts/azworker.sh] most, which spins up a personal Linux VM
87
for you to develop on in the Azure cloud.
88
90
A Git submodule that contains the right version of our dependent libraries.
91
For many years, the Go answer to dependency management was "write
92
backwards-compatible code." That strategy is as dangerous as it sounds, so
93
since v1.6, Go will automatically load packages in a subdirectory named
94
`vendor`, if it exists. See [build/README.md] for details on how we maintain
95
this folder.
96
97
### Other important repositories
98
99
Besides [cockroachdb/cockroach], the [cockroachdb] GitHub organization is
100
home to several other important open-source components of Cockroach:
101
102
* **[cockroachdb/docs]**, which houses the code behind our user-facing
103
documentation at <https://cockroachlabs.com/docs>. At the time of writing, our
104
stellar docs team handles essentially all documentation.
105
106
* **[cockroachdb/examples-go]**, which contains small, self-contained Go
107
programs that exercise CockroachDB via the PGWire protocol. You're likely to
108
hear most about [block_writer], which writes uniformly random values into a
109
table, and [photos], which simulates a more-realistic workload of a
110
photo-sharing site, where some photos and users are orders of magnitude more
111
popular. The other example programs are of a similar scope and purpose, but
112
block_writer and photos are deemed important enough to run constantly against
113
our production clusters.
114
115
* **[cockroachdb/loadgen]**, which contains the next generation of CockroachDB
116
load testing. These include the industry-standard TPC-H and YCSB benchmarks
117
(Google for details), as well as kv, version of block_writer that can target
118
databases other than CockroachDB.
119
120
* **[cockroachdb/examples-orms]**, which showcases ORMs a toy API that uses an
121
ORM to prepare its responses in several different languages.
122
123
Most of the remaining repositories under the [cockroachdb] organization are forks
124
of existing Go libraries with some small, Cockroach-specific patches.
125
126
### Internal documentation
127
128
Documentation on the first-party Go packages that make up CockroachDB is, as of
129
this writing, essentially nonexistent. This is par for the course with code
130
that's evolving as quickly as Cockroach, but it's something we're hoping to
131
improve over time, especially as internal packages stabilize.
132
133
The internal documentation that we *do* have lives in
134
[cockroachdb/cockroach/docs](https://github.com/cockroachdb/cockroach/tree/master/docs).
135
At the time of writing, most of this documentation covers the high-level
136
architecture of the system. Only a few documents hone in on specifics, and even
137
those only cover the features that were found to cause significant developer
138
frustration. For most first-party packages, you'll need to read the source for
139
usage instructions.
140
141
> **Protip:** If you prefer Go-generated HTML documentation to reading the
142
source directly, take a look at
143
<https://godoc.org/github.com/cockroachdb/cockroach>.
144
145
For our internal docs, I recommend the following reading order.
146
147
First, browse through the [design document], which describes the architecture of
148
the entire system at the highest possible level. You'll likely find there's too
149
much information here to digest in one sitting: you should instead strive to
150
remember what topics are covered, so you can refer to it later with more
151
specific questions in mind.
152
153
Then, look through the [docs/tech-notes] folder and determine if any of the tech
154
notes are relevant to your starter project. Again, you'll likely find that the
155
tech notes contain too much information to process, so instead try to identify
156
the sections that are likely to be useful as you make progress on your starter
157
project.
158
159
The one exception to this rule is [docs/tech-notes/contexts.md]. It's worth
160
learning why so many of our function signatures take a context as their first
161
parameter, like so:
162
163
```
164
func doOperation(ctx context.Context, ...)
165
```
166
167
Otherwise, plumbing contexts everywhere will feel like a chore with no upsides.
168
169
Finally, I feel obligated to reproduce this disclaimer from the tech notes README:
170
171
> Standard disclaimer: each document contains parts from one or more author.
172
> Each part was authored to reflect its author's perspective on the project at
173
> the time it was written. This understanding is necessarily subjective: its
174
> context is both the state of the project and the authors', and their
175
> reviewers', experience around that particular date. In case of doubt, consult
176
> your local historian and your repository's timeline.
177
178
In short, our documentation is not authoritative. Trust your reading of the code.
179
180
### Start coding!
181
182
It's time to fire up your editor and get to work! The [CONTRIBUTING.md] document
183
does a good job of describing the basic development commands you'll need. In
184
short, you'll use `make` to drive builds.
185
186
To produce a `cockroach` binary:
187
188
```bash
189
$ make build
190
```
191
192
To run all the tests in `./pkg/rest/of/path`:
193
194
```bash
195
$ make test PKG=./pkg/rest/of/path
196
```
197
198
### Is there a Go debugger?
199
200
Not a good one, sadly. Most of us debug single-cluster issues by littering
201
`log.Infof`s throughout relevant files.
202
203
On Linux, GDB is supposedly reasonably functional; on macOS, building LLDB from
204
source also yields a reasonably-functional debugger. Come talk to Nikhil if you
205
think you'd find this helpful, and we can work on codifying the steps.
206
207
If you're debugging an issue that spans several nodes, you'll want to look into
208
"distributed tracing." Ask your Roachmate to either walk you through
209
OpenTracing/LightStep or point you at someone who can.
210
211
### A note on adding a new file
212
213
You'll notice that all source files in this repository have a license
214
notification at the top. Be sure to copy this license into any files that you
215
create.
216
217
## When to submit
218
219
Code at Cockroach Labs is not about perfection. It's too easy to misinterpret
220
"build it right" as "build it perfectly," but striving for perfection can lead
221
to over-engineered code that took longer than necessary to write. Especially as
222
a startup that moves quickly, building it right often means building the
223
simplest workable solution, then iterating as necessary. What we try to avoid at
224
Cockroach Labs is the quick, dirty, gross hack—but even that can be acceptable
225
to plug an urgent leak.
226
227
So, you should get feedback early and often, while being cognizant of your
228
reviewer's time. You don't want to ask for a detailed review on a rough draft,
229
but you don't want to spend a week heading down the wrong path, either.
230
231
You have a few options for choosing when to submit:
232
233
1. You can open a PR with an initial prototype labeled `[DO NOT MERGE]`. You
234
should also assign the `do-not-merge` GitHub label to your PR. This is useful
235
when you're not entirely sure who should review your code.
236
237
2. Especially on your first PR, you can do everything short of opening a GitHub
238
PR by sending someone a link to your local branch. Alternatively, open a PR
239
against your fork. This won't send an email notification to anyone watching
240
this repository, so it's a good way to let your Roachmate (or the lead
241
reviewer on your project). This works well when you have a reviewer in mind
242
and you want to avoid the onslaught of
243
244
3. For small changes where the approach seems obvious, you can open a PR with
245
what you believe to be production-ready or near-production-ready code. As you
246
get more experience with how we develop code, you'll find that larger and
247
larger changesets will begin falling into this category.
248
249
PRs are assumed to be production-ready (option 3) unless you say otherwise in
250
the PR description. Be sure to note if your PR is a WIP to save your reviewer
251
time.
252
253
If you find yourself with a PR (`git diff --stat`) that exceeds roughly 500 line
254
of code, it's time to consider splitting the PR in two, or at least introducing
255
multiple commits. This is impossible for some PRs—especially refactors—but "the
256
feature is only partially complete" should never be a reason to balloon a PR.
257
258
One common approach used here is to build features up incrementally, even if
259
that means exposing a partially-complete feature in a release. For example,
260
suppose you were tasked with implementing support for a new SQL feature, like
261
supporting subqueries in UPDATE statements. You might reasonably submit four
262
separate PRs. First, you could land a change that adjusts the SQL grammar and
263
links it to a dummy implementation that simply outputs an error like "subqueries
264
in UPDATE not supported" instead of "syntax error." Then, you might land a
265
change to implement a naive version of the feature, with tests to verify its
266
correctness. A third PR might introduce some performance optimizations, and a
267
fourth PR some refactors that you didn't think of until a few weeks later. This
268
approach is totally encouraged! It's no problem if the first PR (the one that
269
simply prints "not implemented") lands in an unstable release, as long as a
270
change isn't backing you into a corner or causing server panics. Code produced
271
from an incremental approach is *much* easier to review, which means better,
272
more robust code.
273
274
### Production-ready code
275
276
In general, production-ready code:
277
278
* Is free of syntax errors and typos
279
280
* Omits accidental code movement, stray whitespace, and stray debugging statements
281
282
* Documents all exported structs and functions
283
284
* Documents internal functions that are not immediately obvious
285
286
* Has a well-considered design
287
288
That said, don't be embarrassed when your review points out syntax errors, stray
289
whitespace, typos, and missing docstrings! That's why we have reviews. These
290
properties are meant to guide you in your final scan.
291
292
## How to submit
293
294
So, you've written your code and written your tests. It's time to send your code
295
for ~slaughter~ review.
296
297
### Fix your style violations
298
299
First, read [docs/style.md] again, looking for any style violations. It's easier to
300
remember a style rule once you've violated it.
301
302
Then, run our suite of linters:
303
304
```bash
305
$ make lint
306
```
307
308
This is not a fast command. On my machine, at the time of writing, it takes
309
about a full minute to run. You can instead run
310
311
```bash
312
$ make lintshort
313
```
314
315
which clocks in at about 30s by omitting the slowest linters.
316
317
### Where to push
318
319
In the interest of branch tidiness, we ask that even contributors with the
320
commit bit avoid pushing directly to this repository. That deserves to be
321
properly called out:
322
323
> **Protip:** don't push to cockroachdb/cockroach directly!
324
325
Instead, create yourself a fork on GitHub. This will give you a semi-private
326
personal workspace at github.com/YOUR-HANDLE/cockroach. Code you push to your
327
personal fork is assumed to be a work-in-progress (WIP), dangerously broken, and
328
otherwise unfit for consumption. You can view @bdarnell's WIP code, for example, by
329
browsing the branches on his fork, <https://github.com/bdarnell/cockroach/>.
330
331
Then, you'll need to settle on a name for your branch, if you haven't already.
332
This isn't a hard-and-fast rule, but Cockroach Labs convention is to
333
hyphenate-your-feature. These branch identifiers serve primarily to identify *to
334
you*.
335
336
To give you a sense, here's a few feature branches that had been merged in
337
recently when this document was written, and their associated commit summaries:
338
339
* knz/default-column: `sql: ensure that DEFAULT exprs are re-evaluated during backfill.`
340
341
* dt/fk: `sql: show only constrained cols in fk`
342
343
It's far more important to give the commit message a descriptive
344
title and message than getting the branch name right.
345
346
### Split your change into logical chunks
347
348
Be kind to other developers, including your future self, by splitting large
349
commits. Each commit should represent one logical change to the codebase with a
350
descriptive message that explains the rationale for the change.
351
[CONTRIBUTING.md] has some advice on writing good commit messages: on your first
352
PR, it's worth re-reading that section to ensure your commit messages follow the
353
guidelines.
354
355
Most importantly, all commits which touch this repository should have the format
356
`pkg: message`, where `pkg` is the name of the package that was most affected by
357
the change. A few engineers still "watch" this repository, which means they
358
receive emails about every issue and PR. Having the affected package at the
359
front of every commit message makes it easier for these brave souls to filter
360
out irrelevant emails.
361
362
> **Protip:** often, you'll realize after a bout of hacking that you've actually
363
> made *n* separate changes, but you've got just one big diff since you didn't
364
> commit any intermediate work. Explaining how to fix this is out of scope for
365
> this guide, but either Git "patch mode" (Google it) or a graphical Git client
366
> will take care of it.
367
368
See also the ["When to submit"] section above for advice on when multiple
369
commits should be further split into multiple PRs.
370
371
### Who to request a review from
372
373
Deciding who should review a PR requires context that you just won't have in
374
your first month. For your starter project, your Roachmate is the obvious
375
choice. For little bugs that you fix along the way, ask your Roachmate or your
376
neighbor if there's someone who's obviously maintaining a particular area of the
377
codebase. (E.g., if you had a question about the code that interfaces with
378
RocksDB, I would immediately direct you to [@petermattis].)
379
380
If you're unable to get a reviewer recommendation, the "Author" listed at the
381
top of the file may be your best bet. You should check that the hardcoded author
382
agrees with the Git history of the file, as the hardcoded author often gets
383
stale. Use `git log FILE` to see all commits that have touched a particular file
384
or directory sorted by most recent first, or use `git blame FILE` to see the
385
last commit that touched a particular line. The GitHub web UI or a Git plugin
386
for your text editor of choice can also get the job done. You might also try
387
`scripts/authors.sh FILE`, which will show you a list of contributors who have
388
touched the file, sorted by the number of commits by that author that touched
389
the file. If there's a clear winner, ask them for a review; if they're not the
390
right reviewer, they'll suggest someone else who is. In cases that are a bit
391
less clear, you may want to note in a comment on GitHub that you're not
392
confident they're the right reviewer and ask if they can suggest someone better.
393
394
If you're still unsure, just ask in chat! You'll usually get a response in under
395
a minute.
396
397
It's important to have a tough reviewer. In the two months I've been here, I've
398
seen a few PRs land that absolutely *shouldn't* have landed, each time because
399
the person who understood why the PR was a bad idea wasn't looped into the
400
review. There's no shame in this—it happens! I say this merely as a reminder
401
that a tough review is a net positive, as it can save a lot of pain down the
402
road.
403
404
Also, note that GitHub allows for both "assignees" and "reviewers" on a pull
405
request. It's not entirely clear what the distinction between these fields is,
406
and we're currently split at Cockroach Labs on what we prefer. Choose the field
407
you like most and move on. Ever since GitHub began auto-suggesting reviewers,
408
there seems to be a slight preference for using the reviewer field instead of
409
the assignee field.
410
411
### Write your PR description
412
413
Nearly everything you're tempted to put in a PR description belongs in a commit
414
message. (As a corollary, nearly everything you put in a commit message belongs
415
in a code comment.)
416
417
You should absolutely if you're looking for a non-complete review—i.e., if your
418
code is a WIP. Otherwise, something simple like "see commit messages for
419
details" is good enough. On PRs with just one commit, GitHub will automatically
420
include that one commit's message in the description; it's totally fine to leave
421
that in the PR description.
422
423
### TeamCity
424
425
We run our own continuous integration server called TeamCity, which runs unit
426
tests and acceptance tests against all open pull requests.
427
428
GitHub displays the status of the latest TeamCity run at the bottom of every
429
pull request. You can click the "Details" link to get insight into a failed
430
build or view real-time status of an in-progress build. Occasionally, a build
431
will fail because of flaky tests. You can verify by running a new build and
432
seeing if the problem disappears; just hit the "Run" button on the top right of
433
the page GitHub links to queue a new build. Less frequently, TeamCity will
434
entirely fail to notice a PR, and GitHub will display "waiting for status to be
435
reported" forever. The ["TeamCity Continuous Integration" wiki page] describes
436
how to fix this.
437
438
Unfortunately, a full tutorial on TeamCity is outside the scope of this
439
document. Ask your Roachmate for an overview if you're confused, or post
440
specific questions in #teamcity.
441
442
That's it! You're done. Sit back and get ready for the impending code review.
443
444
## The impending code review
445
446
We take code reviews very seriously at Cockroach Labs. Please don't be deterred
447
if you feel like you've received some hefty feedback. That's completely normal
448
and expected—and, if you're an external contributor, we very much appreciate
449
your contribution!
450
451
In this repository in particular, merging a PR means accepting responsibility
452
for maintaining that code for, quite possibly, the lifetime of CockroachDB. To
453
take on that reponsibility, we need to ensure that meets our strict standards
454
for [production-ready code].
455
456
No one is expected to write perfect code on the first try. That's why we have
457
code reviews in the first place!
458
459
## By humans for humans
460
461
Probably everyone has at some time had an unpleasant interaction during a code
462
review, and with code reviews being [by humans for humans], one or both of these
463
humans could be having a bad day. As if that weren't enough already, text-based
464
communication is fraught with miscommunication. Consider being a [minimally nice maintainer]),
465
reflect upon mistakes that will inevitably be made, and take inspiration from the
466
constructive and friendly reviews that are the daily staple of our repository.
467
468
### Reviewable
469
470
Except for the smallest of PRs, we eschew GitHub reviews in favor of a
471
third-party app called [Reviewable]. Reviewable has an incredibly dense UI: you
472
could probably master a new musical instrument faster than you could master
473
Reviewable.
474
475
Still, Reviewable is far better than GitHub for large changes that undergo
476
multiple rounds of feedback. The basic model is the same: a diff of your change
477
where your reviewers can leave inline notes. In Reviewable, unlike GitHub, every
478
comment spawns a new thread of discussion, and the UI will highlight any threads
479
that you haven't acknowledged/responded to.
480
481
Reviewable also takes great pains to handle force pushes. When you push a new
482
revision, Reviewable will do its best to match up comment threads to the
483
logically-equivalent location in the new diff. Even if this algorithm fails,
484
Reviewable records each force push as a "revision," and will allow you and your
485
reviewers to track how your PR has evolved as you address review feedback.
486
487
You won't need to do anything special to enable Reviewable for your PR; one of
488
our bots will be along shortly after the PR is opened to post a link to the
489
review interface.
490
491
### Addressing feedback
492
493
If someone leaves line comments on your PR without leaving a top-level "looks
494
good to me" (LGTM), it means they feel you should address their line comments
495
before merging. That is, without an LGTM, all comments are roughly in the
496
discussing disposition, even if the Reviewable disposition marker in the bottom
497
right corner hasn't been set as such. If you agree with their feedback, you
498
should make the requested change; if you disagree, you must leave a comment with
499
your counterargument and wait to hear back.
500
501
If someone leaves a top-level LGTM but also leaves line comments, you can assume
502
all line comments are in the satisfied disposition, even if the Reviewable
503
toggle hasn't been set as such. If you disagree with any of their feedback, you
504
should still let them know (e.g., "actually, I think my way is better because
505
reasons"), but you don't need to wait to hear back from them to merge.
506
507
You may see comments labeled as "optional" to indicate a one-off satisfied
508
disposition when the comment default is discussing (i.e., you haven't yet
509
received an LGTM).
510
511
Similarly, you may see an "LGTM modulo the comments" to indicate that you're
512
free to merge provided that you make the requested changes. If you do disagree
513
with the feedback, then you need to voice your counterargument and wait for the
514
reviewer to respond.
515
516
Rarely, someone will express a sentiment like "I feel very strongly that we
517
shouldn't merge this." Disagreements like these are often easier to resolve
518
outside of Reviewable, via an in-person discussion or via chat.
519
520
As you gain confidence in Go, you'll find that some of the nitpicky style
521
feedback you get does not make for obviously better code. Don't be afraid to
522
stick to your guns and push back. Much of coding style is subjective. As I was
523
learning Go, though, I found it helpful to try all style suggestions. For the
524
first month, essentially every style suggestion I received made for far more
525
idiomatic Go.
526
527
Once you've accumulated a set of changes you need to make to address review
528
feedback, it's time to force-push a new revision. Be sure to amend your prior
529
commits—you don't want to tack on a bunch of fixup commits to address review
530
feedback. If you've split your PR into multiple commits, it can be tricky to
531
ensure your review feedback changes make it into the right commit. A tutorial on
532
how to do so is outside the scope of this document, but the magic keywords to
533
Google for are "git interactive rebase."
534
535
You should respond to all unresolved comments whenever you push a new revision
536
or before you merge, even if it's just to say "Done."
537
538
> **Protip:** Wait to publish any "Done" comments until you've verified that
539
> your changes have been force-pushed and show up properly in Reviewable. It's
540
> very easy to preemptively draft a "Done" comment as you scroll through
541
> feedback, but then forget to actually make the change.
542
543
### CR Jargon
544
545
These are the acronyms you'll frequently encounter during code reviews.
546
547
* **CR**, "code review"
548
* **PR**, "pull request"—you probably knew that one.
549
* **PTAL**, "please take another look"
550
* **RFAL**, "ready for another look"—as in, I made some changes, PTAL.
551
* **LGTM**, "looks good to me"—i.e., ship it!
552
* 👩🔬🐶 , "science dog"—i.e., I have no idea what this code does.
553
* **TF[YT]R**, "thanks for your/the review"
554
555
A list of even more Cockroach jargon lives on [this repository's wiki].
557
### Merging
558
559
*External contributors: you don't need to worry about this section. We'll merge
560
your PR as soon as you've addressed all review feedback!*
561
562
To merge code into any CockroachDB repository, you need at least one LGTM. Some
563
LGTMs matter more than others; you want to make sure you solicit an LGTM from
564
whoever "owns" the code you're touching.
565
566
Occasionally, someone will jump into a review with some minor style nits and
567
leave an LGTM once they see you've addressed the nits; you should still wait for
568
the primary reviewer you selected to LGTM the merits of the design. To confuse
569
matters, sometimes you'll get a drive-by review from someone who was as
570
qualified or more qualified than the primary reviewer. In that case, their LGTM
571
is merge-worthy. Usually it's clear which situation you're dealing with, but if
572
in doubt, ask!
573
574
Once you've gotten an LGTM from who you think is the right person, don't be
575
afraid to merge. The git revert command exists for a reason. You can expect to
576
revert at least one PR that you land in your first three months.
577
578
When your PR is ready to go, request a merge from our build bot Craig. Craig is
579
a [Bors merge bot], whose only job is to be gatekeeper to the repository. Add a
580
comment on the PR of the form `bors r=<reviewer>`, replacing `<reviewer>` with
581
the GitHub username of the person who gave you the LGTM. Once approved, your PR
582
will be batched up with other PRs approved around the same time. Craig will try
583
to build them as though they were merged to the target branch, and if
584
successful, will merge them. This limits your exposure by ensuring you don't
585
accidentally merge a commit with an obviously broken build or merge skew.
586
587
We call merging locally and pushing to master the "nuclear option" or "god
588
merging." It's not disabled, but you shouldn't do it unless the repo is on fire.
589
590
Congratulations on landing your first PR! It only gets easier from here.
591
592
# First PR checklist
593
594
Here's a checklist of action items to keep you sane:
595
596
+ [ ] Developing
597
+ [ ] Create a GitHub fork of cockroachdb/cockroach
598
+ [ ] Read through [CONTRIBUTING.md]
599
+ [ ] Submitting your first PR
600
+ [ ] Push to a feature branch on your personal fork
601
+ [ ] Verify you've followed [CONTRIBUTING.md]
603
+ [ ] Ensure files you've added, if any, have a license block
604
+ [ ] Run make check
605
+ [ ] Split your change into logical commits with good messages
606
+ [ ] Avoid writing a PR description
607
+ [ ] Addressing feedback
608
+ [ ] Amend the appropriate commits and force-push
609
+ [ ] Respond to all feedback with "Done" or a counterargument
610
+ [ ] Merging
612
613
["Building on Linux"]: #building-on-linux
614
["Internal documentation"]: #internal-documentation
615
["TeamCity Continuous Integration" wiki page]: https://github.com/cockroachdb/cockroach/wiki/TeamCity-Continuous-Integration
616
["When to submit"]: #when-to-submit
617
[@bdarnell]: https://github.com/bdarnell
618
[@petermattis]: https://github.com/petermattis
619
[block_writer]: https://github.com/cockroachdb/examples-go/tree/master/block_writer
620
[build/README.md]: /build/README.md
621
[cockroachdb]: https://github.com/cockroachdb
622
[cockroachdb/cockroach]: https://github.com/cockroachdb/cockroach
623
[cockroachdb/docs]: https://github.com/cockroachdb/docs
624
[cockroachdb/examples-go]: https://github.com/cockroachdb/examples-go
625
[cockroachdb/examples-orms]: https://github.com/cockroachdb/examples-orms
626
[cockroachdb/loadgen]: https://github.com/cockroachdb/loadgen
627
[CONTRIBUTING.md]: /CONTRIBUTING.md
628
[design document]: /docs/design.md
629
[docs/tech-notes]: /docs/tech-notes
630
[docs/tech-notes/contexts.md]: /docs/tech-notes/contexts.md
631
[photos]: https://github.com/cockroachdb/examples-go/tree/master/photos
632
[production-ready code]: #production-ready-code
633
[Reviewable]: https://reviewable.io
634
[RocksDB]: http://rocksdb.org
636
[this repository's wiki]: https://github.com/cockroachdb/cockroach/wiki/Jargon
637
[by humans for humans]: https://mtlynch.io/human-code-reviews-1/
638
[minimally nice maintainer]: https://brson.github.io/2017/04/05/minimally-nice-maintainer