Conversation
This replaces the section on the community page. The information from the community page is not only moved into the new contributor's guide, but also updated to the current realities. Plus, a lot of new information is added, like a more helpful introduction for new contributors, a coding style guide, guidelines about handling PRs, and more. I tried to write down what I percieve as the tribal knowledge. However, people might have different versions of this tribal knowledge in their heads, so please discuss during the review what we want to codify here. Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
|
Note that parts of this assume that prometheus/proposals#65 will be accepted. If not the required changes shouldn't be to hard to make, but will require some lengthier descriptions. |
beorn7
left a comment
beorn7
left a comment
There was a problem hiding this comment.
Thank you very much for this epic work. Looks quite good already.
In the same PR, we should update https://github.com/prometheus/docs/blob/main/src/app/community/community.md to not contain its own "Contributing" section anymore but point to this guide. Also, remove all the developer channels from that doc and the section about the dev summit.
Separately from this PR, all the CONTRIBUTING.md files in all repos should be updated to point to this guide.
docs/guides/contributing.md
Outdated
| [`#prometheus-dev:matrix.org`](https://app.element.io/#/room/#prometheus-dev:matrix.org). | ||
| In principle, IRC/Matrix is the preferred chat-like communication channel | ||
| because it is much more open than Slack. However, in practice the traffic on | ||
| IRC is very low. |
There was a problem hiding this comment.
As sad as it is, but "very low" is a euphemism here. For a very long time, the only traffic on #prometheus-dev was spam.
I would either remove the IRC section entirely, or state explicitly that it is effectively unused at the moment (which avoids confusion if people stumble upon the channel).
There was a problem hiding this comment.
I removed it for now. No reason to add more information here that has limited value for the task at hand.
docs/guides/contributing.md
Outdated
| https://developercertificate.org/ for _that_ particular contribution. | ||
|
|
||
| Once you have a change you would like to propose, push it to your personal fork | ||
| of Prometheus and open a pull request against the `main` branch. |
There was a problem hiding this comment.
Maybe here is the right place to explain the subtleties (release branches, master branches).
There was a problem hiding this comment.
I added a little bit, not sure if all subtleties are in there for this context here.
| Reviewers might request changes, which requires the author to either add commits | ||
| or rewrite the existing commits. Prometheus defaults to merging PR commits (not | ||
| rebase or squash), so adding a list of `fixup` commits is not a good idea unless | ||
| they get rebased or squashed by the PR author. Rebase and squash rewrite gits | ||
| commit history and authors should be aware of the implications (for example see | ||
| https://git-scm.com/book/en/v2/Git-Branching-Rebasing). Rebasing a branch for a | ||
| pull request is generally fine. Only once others have changes based on commits | ||
| that are not part of the `main` branch yet, commit authors should refrain from | ||
| rewriting those commits. |
There was a problem hiding this comment.
While I personally agree with the workflow as described here, it is not really representing the common practice in the project.
Many PRs evolve with a ton of fixup commits, and the reviewer squashes them all into one big commit in the end (or asks the author to group the commit into something reasonable if the PR changes different things that should be in separate commits).
I think this is totally fine as a practice, as long as nobody else has based work on the commits in the review branch. And as such, we should represent here as a valid workflow. (We should, though, include here that the reviewer squashing the commits needs to make sure we get a proper commit message, not the autogenerated one you see after hitting "squash" in the GH web UI.)
| that are not part of the `main` branch yet, commit authors should refrain from | ||
| rewriting those commits. | ||
|
|
||
| ## AI generated contributions |
There was a problem hiding this comment.
If we add an AI policy, as discussed on Slack, this section should not duplicate it but simply link to it.
The alternative would be to make this section our AI policy. In that case, we should include (and emphasize) the aspect of the human author being able to understand what they are submitting (see discussion on Slack).
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
|
@jan--f do you want me to do another round of review? Everyone else: It would be extremely helpful if others reviewed this, too. Beyond providing historical context, my opinions here are really the least relevant, given my imminent retirement. |
|
|
||
| If you’re working on your first contribution please read this whole document, this section is aimed at experienced Open Source contributors. | ||
|
|
||
| - Test changes locally and consider adding tests for your change. |
There was a problem hiding this comment.
| - Test changes locally and consider adding tests for your change. | |
| - Test changes locally with `make test` and add tests for your change if possible. |
| If you’re working on your first contribution please read this whole document, this section is aimed at experienced Open Source contributors. | ||
|
|
||
| - Test changes locally and consider adding tests for your change. | ||
| - Commit with `git commit -s` to sign the DCO. |
There was a problem hiding this comment.
I usually run some formatting and linting locally to avoid cycles.
| - Commit with `git commit -s` to sign the DCO. | |
| - To avoid lint failures in CI it's useful to run `make common-format` and `make lint` and fix issues. | |
| - Commit with `git commit -s` to sign the DCO. |
| `#prometheus-dev`, but there are plenty of specialized channels. Look for | ||
| channel names like `#prometheus-...-dev`, e.g. `#prometheus-protobuf-dev`. | ||
|
|
||
| Note that Slack is a silo. The content is not indexed by external search |
There was a problem hiding this comment.
Slack content isn't retained forever afaik, let's mention that as well.
| We have checks that run on every PR. To merge all checks should succeed. Any | ||
| failing checks should be investigated and addressed by the PR author. | ||
|
|
||
| Reviewers might request changes, which requires the author to either add commits |
There was a problem hiding this comment.
Personal preference: I generally don't look at individual commits (unless the author calls it out).
If a PR is too big, I'd rather it's split into multiple PRs. Also if half the PR is renaming things and shuffling things around, and the other half is logic changes, I'm sure to request splitting into an easy to review refactor PR and the substantial PR.
I think it might discourage people from contributing if we're strict about commits. I'd rather we say something on the PR complexity.
| - Use proper English grammar and punctuation. Don’t needlessly abbreviate. | ||
| BAD: // batchQueue full, try again later | ||
| GOOD: // The batchQueue is full, so we need to try again later. | ||
| - In markdown, limit line length to 80 characters, instead of one line per |
There was a problem hiding this comment.
- unless the line ends in a link (URL).
|
|
||
| #### During the summit | ||
|
|
||
| During the summit, the Facilitator is here to make sure the meeting runs |
There was a problem hiding this comment.
| During the summit, the Facilitator is here to make sure the meeting runs | |
| During the summit, the Facilitator is there to make sure that the meeting runs |
3 participants
No description provided.