Skip to content

Commit b402981

Browse files
committed
Address reviews, editorial changes
The biggest change here is the changes to the PR review process. Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
1 parent 88ac994 commit b402981

1 file changed

Lines changed: 48 additions & 42 deletions

File tree

docs/guides/contributing.md

Lines changed: 48 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -10,46 +10,48 @@ levels.
1010
If you are new, you might want to check out [how to pick something to
1111
work on](#how-to-pick-something-to-work-on) first.
1212

13-
If you are an experienced contributor Looking to contribute a change, check the
13+
If you are an experienced contributor looking to contribute a change, check the
1414
[high level process](#how-to-get-a-pr-merged).
1515

1616
If you have concrete plans for something more involved already, please refer to the [proposal
1717
process](#proposal-process).
1818

19-
This document is guide for the prometheus/prometheus repository. Much of this
19+
This document is guide for the `prometheus/prometheus` repository. Much of this
2020
applies to other repos in the Prometheus community, but these projects might
2121
have their own guides and rules. For repository-specific information, check out
2222
the README.md and CONTRIBUTING.md files of each repository.
2323

2424
## How to get a PR merged
2525

26-
If you’re working on your first contribution please read this whole document, this section is aimed at experienced Open Source contributors.
26+
If you’re working on your first contribution, please read this whole document. This section is aimed at experienced open source contributors.
2727

28-
- Test changes locally and consider adding tests for your change.
28+
- Test changes locally with `make test` and add tests for your change if possible.
29+
- To avoid lint failures in CI it's useful to run `make common-format` and
30+
`make lint` and fix issues.
2931
- Commit with `git commit -s` to sign the DCO.
30-
- Generally, open a PRs against the repositories default branch, i.e. most often
32+
- Generally, open a PRs against the repository's default branch, i.e. most often
3133
`main` or sometimes `master`. Reviewers will help with exceptions.
32-
- Reviewers should be assigned automatically, we aim to respond timely but other
34+
- Reviewers should be assigned automatically. We aim to respond in a timely manner, but other
3335
priorities can create delays.
3436
- Check the results of failing CI jobs, all are expected to succeed.
3537
- Reviewers might request changes. Addressing these quickly will improve the
3638
turn-around time of the PR.
3739

38-
For more details around our git refer to
39-
[the Github Guidelines](#github-guidelines).
40+
For more details around our GitHub process, refer to
41+
[the GitHub Guidelines](#github-guidelines).
4042

4143
## Communication channels
4244

4345
The usual [community channels for users](/community) are meant to discuss usage
44-
of Prometheus including using Prometheus code for non-Prometheus development
46+
of Prometheus, including using Prometheus code for non-Prometheus development
4547
(e.g. instrumenting code with a Prometheus instrumentation library).
4648
Development of Prometheus components themselves happens via other channels as
4749
described in this section.
4850

4951
### GitHub
5052

5153
Contributions are reviewed in GitHub pull requests. See the [GitHub
52-
guidelines](github-guidelines) below for details. GitHub issues are often a
54+
guidelines](#github-guidelines) below for details. GitHub issues are often a
5355
good way to discuss specific bugs and feature requests. For informal or
5456
overarching discussions, the other channels below might be more appropriate.
5557

@@ -63,7 +65,8 @@ channel names like `#prometheus-...-dev`, e.g. `#prometheus-protobuf-dev`.
6365
Note that Slack is a silo. The content is not indexed by external search
6466
engines, there is no easy way to export and archive the content, and even
6567
read-only access requires a login. Therefore, consider everything in Slack as
66-
ephemeral and inaccessible for the general public. Important information, like
68+
ephemeral and inaccessible for the general public. Slack content is also not
69+
retained forever. Important information, like
6770
the result of a discussion, should also be published via other channels (e.g.
6871
on [GitHub](github) or the [mailing list](#developer-mailing-list)) to make it
6972
accessible and durable. Avoid linking to Slack messages from other media
@@ -100,18 +103,17 @@ you are already a user or even expert on or something you want to become an
100103
expert on.
101104

102105
The Prometheus community tries to help with identifying good issues to work on
103-
in two way:
106+
in two ways:
104107

105108
1. Look for the label `good-first-issue`. This label identifies work that we
106109
think would be a good starting point for any new contributors.
107110
2. If you are looking for more complex issues to solve look for the
108-
`triage/accepted` label. This label indicates that an issues has been
111+
`triage/accepted` label. This label indicates that an issue has been
109112
triaged, all needed information has been gathered and work can begin.
110113
The labels `triage/needs-triage` and `triage/needs-information` mean that
111114
either no one has had time to look at the issue yet or more information is
112-
need. Do not work on issues labeled `triage/needs-triage` or
113-
`triage/needs-information` .See <label proposal> for more details on how we
114-
use labels.
115+
needed. Do not work on issues labeled `triage/needs-triage` or
116+
`triage/needs-information`.
115117

116118
## Proposal process
117119

@@ -121,7 +123,7 @@ please read the accepted [proposal proposal](https://github.com/prometheus/propo
121123

122124
## GitHub guidelines
123125

124-
Commit message should describe the change made in the commit.
126+
Commit messages should describe the change made in the commit.
125127
We don't have strong opinions on how large or small commits should be, use your
126128
best judgment to make a logical series of changes. Good commits make reviews
127129
easier and thus can be merged faster. For example it's a good pattern to have a
@@ -136,25 +138,26 @@ of Prometheus and open a pull request against the default branch. The default
136138
branch is usually `main` but can be `master` in some repositories.
137139
Some situations require a PR against a release branch, e.g. `release-3.5`. The
138140
most common situations are fixes for a release candidate for a new release or a
139-
fix for a LTS version of Prometheus. If in doubt, ask via one of the channels
141+
fix for an LTS version of Prometheus. If in doubt, ask via one of the channels
140142
mentioned in this document.
141143

142144
The needed reviewers will be added automatically. Anyone else that should or
143145
wants to review a change can be mentioned by their user name in a comment, but
144146
please avoid pinging random community members.
145147

146-
We have checks that run on every PR. To merge all checks should succeed. Any
148+
We have checks that run on every PR. To merge, all checks should succeed. Any
147149
failing checks should be investigated and addressed by the PR author.
148150

149-
Reviewers might request changes, which requires the author to either add commits
150-
or rewrite the existing commits. Prometheus defaults to merging PR commits (not
151-
rebase or squash), so adding a list of `fixup` commits is not a good idea unless
152-
they get rebased or squashed by the PR author. Rebase and squash rewrite gits
153-
commit history and authors should be aware of the implications (for example see
154-
https://git-scm.com/book/en/v2/Git-Branching-Rebasing). Rebasing a branch for a
155-
pull request is generally fine. Only once others have changes based on commits
156-
that are not part of the `main` branch yet, commit authors should refrain from
157-
rewriting those commits.
151+
Reviewers might request changes. It is common practice for PRs to evolve with
152+
additional fixup commits during review. Reviewers may squash these into a
153+
coherent set of commits when merging, but should ensure the final commit
154+
messages are meaningful and not auto-generated. Alternatively, reviewers may
155+
ask the author to group commits into something reasonable before merging.
156+
Rebasing a branch for a pull request is generally fine. Only once others have
157+
changes based on commits that are not part of the `main` branch yet, commit
158+
authors should refrain from rewriting those commits. If a PR becomes too large
159+
or mixes unrelated concerns (e.g. refactoring and logic changes), consider
160+
splitting it into separate PRs to make review easier.
158161

159162
## AI generated contributions
160163

@@ -166,7 +169,8 @@ is based upon previous work, it is covered under an appropriate open source
166169
license and the author has the right under that license to submit that work with
167170
modifications. See https://www.linuxfoundation.org/legal/generative-ai for more
168171
details.
169-
Please consider the DCO and carefully review AI generate code before submitting
172+
The human author must fully understand the code they are submitting. Please
173+
consider the DCO and carefully review AI-generated code before submitting
170174
it. We encourage explicitly disclosing AI tool usage, for example by adding an
171175
`Assisted-by: <name of AI tool used>` to the respective commits.
172176

@@ -183,36 +187,38 @@ Prometheus contributions stick to the following:
183187
- We have plenty of linters, use `make format`, `make lint` and `make style` to
184188
correctly format your code.
185189
- Use proper English grammar and punctuation. Don’t needlessly abbreviate.
186-
BAD: // batchQueue full, try again later
187-
GOOD: // The batchQueue is full, so we need to try again later.
188-
- In markdown, limit line length to 80 characters, instead of one line per
189-
paragraph. It makes commenting in reviews so much easier.
190+
- BAD: `// batchQueue full, try again later`
191+
- GOOD: `// The batchQueue is full, so we need to try again later.`
192+
- In Markdown, limit line length to 80 characters, instead of one line per
193+
paragraph, unless the line ends in a URL. It makes commenting in reviews so
194+
much easier.
190195

191196

192197
### Go style guide
193198

194199
Go is the main programming language used in Prometheus and its ecosystem. Go
195-
based project tend to follow a very similar style, Prometheus is no exception.
200+
based projects tend to follow a very similar style, Prometheus is no exception.
196201
https://go.dev/wiki/CodeReviewComments is a great resource for specifics. We
197-
have a few rules that are worth mentioning here:
202+
have a few rules on top of that worth mentioning here:
198203

199204
- Often we put named imports into a separate block. The blocks should be grouped
200205
into stdlib / other repos / same repo.
201206
- Doc comments on exported types are not enforced by the linter (because of too
202207
many false positives), but we do care. Use them where they make sense.
203-
- For long function signatures that require line breaks put closing parentheses
204-
on separate line. For example:
205-
```
208+
- Break long function signatures into multiple lines in the following way:
209+
End the first line with the opening parenthesis, then put the function
210+
parameters on any number of lines as you see fit, followed by a separate
211+
line beginning with the closing parenthesis. For example:
212+
```go
206213
func (s *shards) sendSamples(
207214
ctx context.Context, samples []prompb.TimeSeries,
208215
sampleCount, exemplarCount, histogramCount int,
209216
pBuf *proto.Buffer, buf compression.EncodeBuffer, compr compression.Type,
210217
) error {
211218
```
212219
**NOT**
213-
```
214-
func (s *shards) sendSamples(
215-
ctx context.Context, samples []prompb.TimeSeries,
220+
```go
221+
func (s *shards) sendSamples(ctx context.Context, samples []prompb.TimeSeries,
216222
sampleCount, exemplarCount, histogramCount int,
217223
pBuf *proto.Buffer, buf compression.EncodeBuffer, compr compression.Type) error {
218224
```
@@ -280,7 +286,7 @@ attend the summit. We suggest the following tasks:
280286
281287
#### During the summit
282288
283-
During the summit, the Facilitator is here to make sure the meeting runs
289+
During the summit, the Facilitator is there to make sure that the meeting runs
284290
smoothly, and that consensus is reached when needed. We suggest the
285291
following tasks:
286292

0 commit comments

Comments
 (0)