Skip to content
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

Jump to bottom

docs: Add contributing guidelines for go-tuf #190

Closed
wants to merge 1 commit into from
Closed

docs: Add contributing guidelines for go-tuf #190

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
61 changes: 60 additions & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,60 @@
See the [Flynn contributing guide](https://flynn.io/docs/contributing).
# Contributing Guide

We welcome and encourage community contributions to go-tuf.

Please familiarize yourself with the Contribution Guidelines before contributing.

There are many ways to help go-tuf besides contributing code:
- Fix bugs or file issues
- Provide feedback on the CLI experience or suggest feature enhancements.
- Improve documentation.

## Contributing Code

Unless you are fixing a known bug, we strongly recommend discussing it with the community via a GitHub issue or Slack before getting started to ensure that your work is consistent with TUF's specification.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe: ...or Slack (see [Communication](#Communication))


All contributions are made via pull request. All patches from all contributors get reviewed. See the Pull Request procedure.


## Pull Request Procedure

To make a pull request, you will need a GitHub account. See GitHub's documentation [forking](https://help.github.com/articles/fork-a-repo) and [pull requests](https://help.github.com/articles/using-pull-requests).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "Github's documentation ON forking..."


Pull requests should be targeted at the `master` branch. Before creating a pull request, go through this checklist:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section is a great addition, especially for new contributers


1. Create a feature branch off of `master` so that changes do not get mixed up.
2. If your PR adds new code, it should include tests covering the new code. If your PR fixes a bug, it should include a regression test.
3. PRs that change user-facing behavior or CLI must have associated documentation.
4. All code comments and documentation are expected to have proper English grammar and punctuation.
5. [Rebase](http://git-scm.com/book/en/Git-Branching-Rebasing) your local changes against the `master` branch.
6. Run the full project test suite with the `go test ./...` command and confirm that it passes.
7. Run `go fmt ./...`.

When creating a PR,

1. Accept the Developer's Certificate of Origin on all commits (see above).
Copy link
Contributor

Choose a reason for hiding this comment

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

If we require this, probably worth adding the DCO bot

Copy link
Member

@trishankatdatadog trishankatdatadog Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I hate that DCO bot. Can we instead use an app that checks whether a contributor has signed a DCO CLA agreement? Something like what the Linux Foundation uses.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤣 🤣 I'm not attached to any particular bot, just saying that if it's "required" that should be enforced :)

Though I can't find what you're talking about -- I checked a few LF repos and the ones I found all use the same bot (ex.).

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant a CLA bot, which many orgs have their own apps for, here's the LF one: https://github.com/apps/lf-cla-application

Copy link
Member

Choose a reason for hiding this comment

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

TLDR: no to DCO, yes to CLA

Copy link
Member

Choose a reason for hiding this comment

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

Why the preference for CLA?

As I understand it (I am not a lawyer) both are assertions that the submitter has the right to submit a change and grants the right to the receiver to use the submitted change. A CLA is a legal contract while the DCO is a more lightweight statement of the contributor asserting ownership. As a consequence of a CLA being a legal contract, they can be a barrier to contributors (requiring, i.e., a corporate legal representative to sign-off).

Copy link
Member

Choose a reason for hiding this comment

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

Simply because DCO is extremely annoying (must remember to --sign-off, otherwise rebase and force-push hell; does not work with commits done from the website), whereas CLA is a one-time deal.

2. Your PR title should be descriptive, and generally start with a subsystem prefix (ex: `client: `).
3. Your PR commit message will be used as the commit message when your PR is merged. Update this field if your PR diverges during review.
4. Your PR description should have details on what the PR does. If it fixes an existing issue, include a line like "Fixes #XXXX".

When all of the tests are passing, maintainer(s) will be assigned to review and merge the PR.


## Communication
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be nice to mention the community meeting schedule as well 👍


We use the [#tuf](https://cloud-native.slack.com/archives/C8NMD3QJ3) and [#go-tuf](https://cloud-native.slack.com/archives/C02D577GX54) channel on [CNCF Slack](https://slack.cncf.io/). You are welcome to drop in and ask questions, discuss bugs, etc.

## Pull Request review policy

* Anyone is welcome to review any PR, whether they are a maintainer or not!
* Maintainers should aim to turn around reviews within one business day.
Copy link
Collaborator

Choose a reason for hiding this comment

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

One day might be a bit optimistic. While this will usually be the case maintainers might take a week off, etc and I don't want to set expectations too high.

Copy link
Member

Choose a reason for hiding this comment

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

How does a 5 business days sound? Also, it's unlikely all the maintainers are off, and I think we can make exceptions for regional holidays.

* See [MAINTAINERS](MAINTAINERS) for the current list of maintainers.
* It is expected that two maintainers from differing organizations approve the PR before a merge. This may be waived for PRs which only update docs or comments, or trivial changes to tests.

Maintainers should:
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can also have something here about ensuring compliance with the TUF spec and overall code quality

* Make sure that the PR title, commit message, and description are updated if the PR changes significantly during review.
* Ensure that the PR guidelines above are satisfied (tests are added, documentation is added, etc).