Skip to content

Latest commit

 

History

History
312 lines (214 loc) · 15.7 KB

CONTRIBUTING.md

File metadata and controls

312 lines (214 loc) · 15.7 KB

Contributing Guidelines

​:+1::tada: First off, thanks for taking the time to contribute! :tada::+1:

The following is a set of guidelines for contributing to the Filecoin Project. Feel free to propose changes, as this is a living document.

Filecoin, including go-filecoin and all related modules, follows the Filecoin Code of Conduct.

Table Of Contents

How can I contribute?

Here at go-filecoin, there’s always a lot of work to do. There are many ways you can support the project, from progamming, writing, organizing, and more. Consider these as starting points:

  • Submit bugs: Perform a cursory search to see if the problem has already been reported. If it does exist, add a 👍 to the issue to indicate this is also an issue for you, and add a comment if there is extra information you can contribute. If it does not exist, create a new issue (using the Bug report template).

  • Write code: Once you've read this contributing guide, check out Good First Issues for well-prepared starter issues.

  • Improve or write documentation: Docs live at filecoin-project/docs.

  • New ideas: Open a thread on the Discussion Board.

What should I know before getting started?

Check out the Go-Filecoin code overview for a brief tour of the code.

Design Before Code

  • Write down design intent before writing code, and subject it to constructive feedback.
  • Major changes should have a Design Doc.
  • For minor changes, file an issue for your change if it doesn't yet have one, and outline your implementation plan on the issue.

Pull Requests and Reviews

We review every change before merging to master, using GitHub pull requests. Try to keep PRs small, no more than 400 lines or 8 files. Code reviews are easier, faster, and more effective when the diff is small.

Merging a PR to master requires maintainer approval. The following process aims to merge code quickly and efficiently while avoiding both accidental and malicious introduction of bugs, unintended consequences, or vulnerabilities.

  1. Committers require approval from any single maintainer, before landing their own PRs (maintainers require approval from any committer).
  2. Non-committers require approval first from a committer familiar with the relevant area of code. Once they deem it ready for merge, the reviewer will assign a maintainer for approval.
    • Once approved, the reviewing committer is responsible for landing the commits.

If your PR hasn't been reviewed in 3 days, pinging reviewers via Github or community chat is welcome and encouraged.

We use the following conventions for code reviews:

  • "Approve" means approval. If there are comments and approval, it is expected that you'll address the comments before merging. Ask for clarification if you're not sure.

    • Example: reviewer points out an off by one error in a blocking comment, but Approves the PR. Reviewee must fix the error, but PR can progress to maintainer review. Committer confirms this before merge.

  • "Request Changes" means you don't have approval, and the reviewer wants another look.

    • Example: the whole design of an abstraction is wrong and reviewer wants to see it reworked.

  • By default, code review comments are advisory: the reviewee should consider them but doesn't have to respond or address them. Comments that start with "BLOCKING" must be addressed and responded to. If a reviewer makes a blocking comment but does not block merging (by marking the review "Add Comments" or "Approve") then the reviewee can merge if the issue is addressed.

In rare cases, a maintainer may request approval from all maintainers for a wide-reaching PR.

Reviewer Responsibilities:

Avoid lengthy design discussions in PR reviews. Major design questions should be addressed during the Design Before Code step. If the conversation snowballs, prefer to merge and spin out an issue for discussion, and then follow up on the process failure that led to needing a big design discussion in a PR.

It is considered helpful to add blocking comments to PRs that introduce protocol changes that do not appear in the spec.

Landing Changes

We strongly discourage merge commits on the master branch. When merging to master:

  • squash your PR's commits into one commit with an encompassing message, and
  • rebase against master so that your commit lands as a "fast-forward", without a merge commit.

At the command line, update your branch with git fetch origin +master:master; git rebase master. "Merge" your changes with git checkout master; git merge --ff-only <mybranch>, and push.

On GitHub, use the grey "updated branch" button, which adds a merge commit to your branch, but then land your changes with the "rebase and merge" or "squash and merge" options on the green merge button. Both of these will rebase your branch, squashing out any trailing merge commits in the process.

We may enable GitHub branch protections requiring that a CI build has passed on the branch, that the branch is up to date with master, or both. If either protection is not in force, committers should use their best judgement when landing an untested commit.

Issues and tracking

We use GitHub issues to track all significant work, including design, implementation, documentation and community efforts. We also use ZenHub to record issue priority and track team progress. ZenHub adds some useful project management overlay data to GitHub issues.

Good First Issues

Ready to begin? Here are well-prepared starter issues (E-good-first-issue) for your coding pleasure. They have clear problem statements, pointers to the right areas of the code base, and clear acceptance criteria.

To pick up an issue:

  1. Assign it to yourself.
  2. Ask for any clarifications via the issue, pinging in community chat as needed.
  3. For issues labeled PROTOCOL BREAKING see the spec section for additional instructions.
  4. Create a PR with your changes, following the Pull Request and Code Review guidelines.

For continued adventures, search for issues with the label E-help-wanted. These are slightly thornier problems that are also reasonably well-prepared.

Pipelines (ZenHub)

We use ZenHub pipelines to track the flow of work on open issues. This usage can vary over time, but the pipelines roughly indicate:

  • Inbox: new issues yet to be triaged
  • Candidates: reasonably complete issues, ready for prioritisation by maintainers
  • Icebox: no near-term intention for implementation
  • Backlog: not an immediate priority, but likely to become one in the future
  • Ready: ready for implementation
  • In Progress: actively being worked on
  • Review/QA: in review (usually triggered by linking a PR)
  • Blocked: unable to progress

Committers and maintainers are expected to keep pipeline information up to date, including on behalf of contributors whose work they are shepherding. Other contributors do not need to maintain this state.

ZenHub can also track dependencies between issues, which is encouraged.

Labels (GitHub)

We use GitHub issue labels to aid browsing, search and discovery of issues related to some outcome or theme. We use a relatively small set of labels, which doesn't change very frequently. Labels are used inclusively, to aid discovery. An issue may have multiple labels; not all issues are expected to have labels. A label is never expected to be "done" (stable state of no open issues).

Labels mark dimensions including:

  • Area (name prefixed with A-): an area of code functionality
  • Category (name prefixed with C-): type of issue, e.g. bug, tech debt, ux
  • Engagement (name prefixed with E-): issues suitable for broader community involvement
  • Importance (name prefixed with I-): highlights why issues are important, e.g. related to security, scale, or runtime crashes
  • Priority (P0 thru P3): not consistently used across the repo; may be used within epics or areas
  • protocol breaking: resolution of the issue will break compatibility with previous versions of the code
  • ...plus a few other binary tags

Epics (ZenHub)

An epic is a set of issues required to deliver a particular outcome, typically designed in a design doc. Epics support focus and forecasting through exclusion or inclusion of (ir-)relevant issues. We will typically scope an epic to a deliverable targeting a particular release, so most epics should live less than six weeks.

Releases (ZenHub)

Release tags identify issues targeted to or blocking a particular go-filecoin software release. Release tags are forward-looking and support forecasting and focus though inclusion or exclusion of issues. At present (April 2019) we aim for a time-based release roughly every six weeks. ZenHub release tags can span multiple repositories.

Milestones (GitHub)

We don't actively use GitHub milestones (but have in the past). Milestones support short time-based cycles such as sprints. Milestones are repo-specific.

Roles

There are four main roles for people participating in go-filecoin. Each has a specific set of abilities and responsibilities: Contributors, Collaborators, Committers, and Maintainers.

Contributors

Anyone is welcome. If you have created an issue, commented on an issue or discussion forum thread, or submitted a PR, you are a contributor.

Responsibilities:

Abilities:

  • Open issues and PRs
  • Comment on issues and PRs

Collaborators

A Collaborator is someone who demonstrates helpful contributions to the project.

Responsibilities:

  • Make helpful contributions via issues, PRs, and other venues

Abilities:

  • Write to the repo (but cannot merge to master)
  • Manage issues

Committers

A Committer is someone with a broad understanding of the codebase and the judgment and humility to call on others' expertise when needed. They have a consistent track record of quality contributions, regular participation, and enabling others.

Responsibilities:

  • Review PRs and guide work to ready-to-merge
  • With maintainer approval, rebase and merge contributor PRs
  • Issue triage and other project stewardship

Abilities:

  • Manage issues
  • Merge PRs

Maintainers

A Maintainer is someone:

  1. who is invested in and broadly familiar with the project, as demonstrated by a history of significant technical-, process-, and/or project-level contributions;
  2. who deeply understands the system, especially knowing when and who to defer to as a reviewer, and with an eye towards unintended consequences;
  3. who is actively engaged in project progress and stewardship by enabling others through project-wide planning, code reviews, design feedback, etc.; and
  4. who is a model of trustworthiness, technical judgement, civility, and helpfulness.

Currently, go-filecoin maintainers are (alphabetically): @acruikshank, @anorth, @whyrusleeping and @zenground0. They can be mentioned in issues and PRs by tagging @filecoin-project/go-filecoin-maintainers.

Responsibilities:

  • Review: Timely, friendly review of PRs and design docs to ensure high-quality code and grow knowledge of committers and frequent contributors
  • Planning and Improvements: Participate meaningfully in technical and process-related improvements at the project level
  • Make significant, direct technical contributions
  • Backstop for hard problems and general project stewardship (TODO: improve wording)

Abilities:

  • Manage issues
  • Merge PRs

Becoming a committer or maintainer: Anyone can nominate someone for committership or maintainership by filing an issue pointing to evidence that the candidate (1) meets the definition and (2) is already performing the responsibilities described in Roles. Existing maintainers must unanimously approve the new candidate. Removing a committer or maintainer requires either self-nomination, or confirmation by at least 66% of existing maintainers.

Additional Developer Notes

Testing

  • All new code should be accompanied by unit tests. Prefer focussed unit tests to integration tests for thorough validation of behaviour. Existing code is not necessarily a good model, here.
  • Integration tests (in-process, daemon, or FAST) should test integration, not comprehensive functionality
  • Tests should be placed in a separate package, and follow the naming pattern $PACKAGE_test. For example, a test of the chain package should live in a package named chain_test. In limited situations, exceptions may be made for some "white box" tests placed in the same package as the code it tests.

Profiling

We use pprof to capture and visualize performance metrics.

To capture (for example) a CPU profile, launch the daemon and then make an HTTP request of the following form:

curl 'http://localhost:${CMDAPI_PORT}/debug/pprof/profile?seconds=15' > /tmp/profile.dump

Then, use pprof to view the dump:

go tool pprof /tmp/profile.dump

Conventions and Style

We use the following import ordering.

import (
        [stdlib packages, alpha-sorted]
        <single, empty line>
        [external packages]
        <single, empty line>
        [go-filecoin packages]
)

Where a package name does not match its directory name, an explicit alias is expected (goimports will add this for you).

Example:

import (
	"context"
	"testing"

	cmds "github.com/ipfs/go-ipfs-cmds"
	cid "github.com/ipfs/go-cid"
	ipld "github.com/ipfs/go-ipld-format"
	"github.com/stretchr/testify/assert"

	"github.com/filecoin-project/go-filecoin/internal/pkg/testhelpers"
	"github.com/filecoin-project/go-filecoin/internal/pkg/types"
)