add CONTRIBUTING guide #1071
add CONTRIBUTING guide #1071
Conversation
|
@aboch lets get this one in. hopefully it will help with PRs format :) |
define contributing guide with general guidelines. Signed-off-by: adrianc <adrianc@nvidia.com>
0d04bf0 to
f12aff6
Compare
|
|
||
| ## General | ||
|
|
||
| * One commit per PR. |
There was a problem hiding this comment.
I really have a big issue with this; yes, a PR should not have "fix-up" commits, but I don't see a good reasons for making a single-commit-per-PR a requirement; also see my comment on #1048 (comment)
Commits should be logically grouped (and each commit in a PR should build), but a pull request may contain multiple commits to make them useful.
There was a problem hiding this comment.
As the creator of the initial policy, the thing I care about is making sure the history is linear and every commit can build properly. The easiest way to enforce this is to disallow merge commits and require one commit per pr. I think a series of commits that can be rebased cleanly onto main is also fine although I find that is a bit harder for newer contributors to do well.
There was a problem hiding this comment.
My goal with this PR is to reflect the current policies defined by the maintainers of the repo so we can have a baseline.
personally i usually do a merge commit or squash and merge in other GH repos where im a maintainer.
TBH i see far too often PRs with multiple commits that have really no business getting merged as is.
e.g
a PRs that end up having something of the sort of (each bullet is a commit, stack style bottom to top):
- forgot to cover some scenario via test
- fix that lint issue
- address review comments
- feat: initial implementaion
There was a problem hiding this comment.
Right, but that's part of the review process. I have definitely dealt with a good share of contributions in Moby (Docker), runc, and Containerd where the PR has an unclean set of commits ("add foo, fix previous commit, ..."), and for those, part of the review is to come to a good structure for the commits (which could be "these all belong in a single one")
But there's many examples where a carefully crafted set of commits, all part of the related units of work, provide much value. They may be part of a larger unit of work but help describe the change made to provide context, or to separate more mundane changes from the important bits of the work, which can provide value during review, but also when having to revisit a change (blame will provide the correct context). Just picking a random example; opencontainers/runc#3216
There was a problem hiding this comment.
As the creator of the initial policy, the thing I care about is making sure the history is linear and every commit can build properly. The easiest way to enforce this is to disallow merge commits and require one commit per pr. I think a series of commits that can be rebased cleanly onto main is also fine although I find that is a bit harder for newer contributors to do well.
Exactly. This is the best way to guarantee each commit is self-contained. CI passes. It can be cleanly ported to other branches. It forces the author to push a finalized, tested change.
@thaJeztah In the past nine years, I very rarely saw a PR with multiple commits here. It's just a non-issue to discuss about, IMO.
define contributing guide with general guidelines.