docs: Add Committer Expectations document #52731
Conversation
|
Marked as a draft, but this is ready for feedback. Once the content of this doc has consensus, I'll also update the guidlines.rst to remove redundant/obsolete info and link to this doc. |
|
Process group issue: #50836 |
There was a problem hiding this comment.
Overall seems reasonable. I know this is draft, but I'll leave some nitpicks alongside substantive points just in case.
Are you planning on reorganizing other documents too to make them easier to find? Or when you take this out of draft, will it still be mainly building on top of existing docs?
Thanks for this!
| is not enforced by the CI system, so it is the PR author’s responsibility to | ||
| verify. | ||
|
|
||
| - When major new functionality is added, tests for the new functionality MUST be |
There was a problem hiding this comment.
Wording nit: This draft contains a fair amount of passive voice, and you could probably improve it by changing some to active voice.
mbolivar-nordic
requested review from
stephanosio,
simhein,
fabiobaltieri,
teburd,
nashif and
dleach02
keith-zephyr
force-pushed
the
committer-expectation
branch
from
e54f67b to
d255999
Compare
There was a problem hiding this comment.
I generally love that we are trying to write this down. There needs to some compromise though I think given many contributions and contributors come from using zephyr to finding a bug and submitting a small update. These contributors should be encouraged with easy to submit and contribute changes with as few barriers to entry as possible.
keith-zephyr
force-pushed
the
committer-expectation
branch
3 times, most recently
from
e794611 to
5d67ef7
Compare
| - PRs must pass all CI checks. This is a requirement to merge the PR, but you | ||
| may mark a PR as draft and explicitly request reviewers to provide early |
There was a problem hiding this comment.
One edge case that you may want to consider here is that first-time contributors require an org member to approve the CI run for their PRs. This is relevant to this situation because people tend to ignore drafts. This creates a possibility for a PR to never exit draft because it never gets CI results because maintainers won't approve the run because it's a draft. Catch 22.
There was a problem hiding this comment.
first-time contributors require an org member to approve the CI run for their PRs.
While a little documentation never hurts, does the "user-interface" make this and the next step(s) clear? It really should.
| - Incompatible changes to APIs must also update the release notes for the | ||
| next release detailing the change. APIs marked as experimental are excluded |
There was a problem hiding this comment.
This will be worth highlighting to maintainers; we don't do this now.
keith-zephyr
force-pushed
the
committer-expectation
branch
from
5d67ef7 to
16bdac2
Compare
keith-zephyr
dismissed stale reviews from jhedberg, carlescufi, fabiobaltieri, and alevkoy
via
299ded4
keith-zephyr
force-pushed
the
committer-expectation
branch
from
0d05a3d to
299ded4
Compare
|
Note - commit 299ded4 only renames the filename committer_expectations to contributor_expectations. I have another revision to push that will include resolution to the open change requests. |
keith-zephyr
force-pushed
the
committer-expectation
branch
from
299ded4 to
bd85461
Compare
Collect up all the contributor expectations and PR requirements into a single place. Add additional guidelines about creating small PRs and how to break up PRs into multiple commits. Signed-off-by: Keith Short <[email protected]>
The RFC proposal documentation better belongs with the other documentation related to contributing to the Zephyr project. Signed-off-by: Keith Short <[email protected]>
keith-zephyr
force-pushed
the
committer-expectation
branch
from
bd85461 to
e443319
Compare
carlescufi
requested review from
kartben,
rerickson1,
ElectricalPaul,
teburd,
nashif and
jhedberg
| reviewers. | ||
|
|
||
| - As GitHub does not implement |git range-diff|_, try to minimize rebases in the | ||
| middle of a review. If a rebase is required, push this as a separate update |
There was a problem hiding this comment.
As GitHub does not implement |git range-diff|_, try to minimize rebases in the middle of a review.
Could an admin please disable the "Update branch" button that shows up in every PR?
It's apparently just one checkbox:
https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-suggestions-to-update-pull-request-branches
There was a problem hiding this comment.
Could an admin please disable the "Update branch" button that shows up in every PR?
Done
Collect up all the committer expectations and PR requirements into a single place. Add additional guidelines about creating small PRs and how to break up PRs into multiple commits.
Signed-off-by: Keith Short [email protected]