Skip to content

add CONTRIBUTING guide #1071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+41 −0
Open

add CONTRIBUTING guide #1071

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Contribution Guidelines

netlink accepts contributions via GitHub pull request(PR). This document outlines some of the conventions on how to contribute
code to the project.

## General

* One commit per PR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@adrianchiris adrianchiris Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* A PR covers a single area/functionality (e.g extension of devlink command and link command should go in separate PRs).
* Each PR should be covered by unit tests when applicable.
* The goal of Netlink package is to follow iproute2 behavior. When in doubt refer to iproute2 [source code](https://github.com/iproute2/iproute2).

## Code Style

Please follows the standard formatting recommendations and language idioms set out in [Effective Go](https://golang.org/doc/effective_go.html)
and in the [Go Code Review Comments wiki](https://github.com/golang/go/wiki/CodeReviewComments).

## Commit Message Style

Each commit is expected to comply with the following format:

```
Change summary

More detailed explanation of your changes: Why and how.
Wrap it to 72 characters.
See [here] (https://chris.beams.io/posts/git-commit/)
for some more good advices.
```

For example:

```
Fix poorly named identifiers

One identifier, fnname, in func.go was poorly named. It has been renamed
to fnName. Another identifier retval was not needed and has been removed
entirely.
```

> __Note:__ [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summary) style is not enforced however is allowed