Skip to content

Latest commit

 

History

History
324 lines (223 loc) · 17.5 KB

CONTRIBUTING.md

File metadata and controls

324 lines (223 loc) · 17.5 KB
Raw

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 venus and all related modules, follows the Filecoin Code of Conduct.

Table Of Contents

How can I contribute?

Here at venus, 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 venus 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.
  • For a major change, draft a Design Doc outlining the context, goals, and your proposed solution.
  • For less a significant change, outline the proposed change in a GitHub issue. If there's already an issue requesting the change, a comment there suffices.
  • Smaller changes, like simple, uncontroversial improvements, don't require an issue. A PR description outlining the motivation and approach is enough. Incremental steps towards a larger outcome don't require individual issues; reference the motivating issue and note each PR achieves.

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 new/changed lines. Code reviews are easier, faster, and more effective when the diff is small.

Strive to separate refactors, renames, and significant clean-ups from changes in functionality. Break a large effort into multiple smaller PRs. Each such PR should be self-consistent, well-tested, and a clear step forward, but need not be a complete implementation of a large feature. Sometimes other strategies (like an integration branch) make more sense; propose this to your reviewers beforehand.

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 venus 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 venus. 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

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

Imports

We use the following import ordering.

import (
        [stdlib packages, alpha-sorted]
        <single, empty line>
        [external packages]
        <single, empty line>
        [venus 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/venus/internal/pkg/testhelpers"
	"github.com/filecoin-project/venus/internal/pkg/types"
)

Comments

Comments are a communication to other developers (including your future self) to help them understand and maintain code. Good comments describe the intent of the code, without repeating the procedures directly.

  • A TODO: comment describes a change that is desired but could not be immediately implemented. It should to include a reference to a GitHub issue outlining whatever prevents the thing being done now (which could just be a matter of priority).
  • A Note: comment indicates an aside, some background info, or ideas for future improvement, rather than the intent of the current code. It's often fine to document such ideas alongside the code rather than an issue (at the loss of a space for discussion).
  • A Dragons: comment marks a piece of code under active development or refactor that is not well structured, expressed, or documented and thus may be difficult to work with. Such comments are intended to live for only a week or two; long-lived technical debt is better documented in an issue.
  • FIXME, HACK, XXX and similar tags indicating that some code is less than perfect should be avoided in favour of TODO, NOTE or some straight prose.