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.
| 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).
There was a problem hiding this comment.
Left this in. While we have an AGENT.md now, it is/should really be aimed at the agents. We can always change things if our AI policy changes or moves somewhere.
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. |
docs/guides/contributing.md
Outdated
| 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.
juliusv
left a comment
juliusv
left a comment
There was a problem hiding this comment.
A set of mostly editorial changes.
Otherwise, good approach.
docs/guides/contributing.md
Outdated
|
|
||
| ## How to get a PR merged | ||
|
|
||
| If you’re working on your first contribution please read this whole document, this section is aimed at experienced Open Source contributors. |
There was a problem hiding this comment.
| If you’re working on your first contribution please read this whole document, this section is aimed at experienced Open Source contributors. | |
| If you’re working on your first contribution, please read this whole document. This section is aimed at experienced open source contributors. |
| func (s *shards) sendSamples( | ||
| ctx context.Context, samples []prompb.TimeSeries, | ||
| sampleCount, exemplarCount, histogramCount int, | ||
| pBuf *proto.Buffer, buf compression.EncodeBuffer, compr compression.Type) error { |
There was a problem hiding this comment.
Could also be like this (as a NOT example):
| func (s *shards) sendSamples( | |
| ctx context.Context, samples []prompb.TimeSeries, | |
| sampleCount, exemplarCount, histogramCount int, | |
| pBuf *proto.Buffer, buf compression.EncodeBuffer, compr compression.Type) error { | |
| func (s *shards) sendSamples(ctx context.Context, samples []prompb.TimeSeries, | |
| sampleCount, exemplarCount, histogramCount int, | |
| pBuf *proto.Buffer, buf compression.EncodeBuffer, compr compression.Type) error { |
Maybe add this as an additional example?
The biggest change here is the changes to the PR review process. Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
4 participants
No description provided.