ooni-probe-cli/CONTRIBUTING.md

168 lines
7.6 KiB
Markdown
Raw Normal View History

# Contributing to ooni/probe-cli
This is an open source project, and contributions are welcome! You are welcome
to open pull requests. An open pull request will be reviewed by a core
developer. The review may request you to apply changes. Once the assigned
reviewer is satisfied, they will merge the pull request.
## OONI Software Development Guidelines
Please, make sure you read [OONI Software Development Guidelines](
https://ooni.org/post/ooni-software-development-guidelines/). We try in
general to follow these guidelines when working on ooni/probe-cli. In
the unlikely care where those guidelines conflict with this document, this
document will take precedence.
## Opening issues
Please, before opening a new issue, check whether the issue or feature request
you want us to consider has not already been reported by someone else. The
issue tracker is at [github.com/ooni/probe/issues](https://github.com/ooni/probe/issues).
## PR requirements
Every pull request that introduces new functionality should feature
comprehensive test coverage. Any pull request that modifies existing
functionality should pass existing tests. What's more, any new pull
request that modifies existing functionality should not decrease the
existing code coverage.
Long-running tests should be skipped when running tests in short mode
using `go test -short`. We prefer internal testing to external
testing. We generally have a file called `foo_test.go` with tests
for every `foo.go` file. Sometimes we separate long running
integration tests in a `foo_integration_test.go` file.
If there is a top-level DESIGN.md document, make sure such document is
kept in sync with code changes you have applied.
Do not submit large PRs. A reviewer can best service your PR if the
code changes are around 200-600 lines. (It is okay to add more changes
afterwards, if the reviewer asks you to do more work; the key point
here is that the PR should be reasonably sized when the review starts.)
In this vein, we'd rather structure a complex issue as a sequence of
small PRs, than have a single large PR address it all.
As a general rule, a PR is reviewed by reading the whole diff. Let us
know if you want us to read each diff individually, if you think that's
functional to better understand your changes.
## Code style requirements
Please, use `go fmt`, `go vet`, and `golint` to check your code
contribution before submitting a pull request. Make sure your code
is documented. At the minimum document all the exported symbols.
Make sure you commit `go.mod` and `go.sum` changes. Make sure you
run `go mod tidy` to minimize such changes.
refactor: flatten and separate (#353) * refactor(atomicx): move outside the engine package After merging probe-engine into probe-cli, my impression is that we have too much unnecessary nesting of packages in this repository. The idea of this commit and of a bunch of following commits will instead be to reduce the nesting and simplify the structure. While there, improve the documentation. * fix: always use the atomicx package For consistency, never use sync/atomic and always use ./internal/atomicx so we can just grep and make sure we're not risking to crash if we make a subtle mistake on a 32 bit platform. While there, mention in the contributing guidelines that we want to always prefer the ./internal/atomicx package over sync/atomic. * fix(atomicx): remove unnecessary constructor We don't need a constructor here. The default constructed `&Int64{}` instance is already usable and the constructor does not add anything to what we are doing, rather it just creates extra confusion. * cleanup(atomicx): we are not using Float64 Because atomicx.Float64 is unused, we can safely zap it. * cleanup(atomicx): simplify impl and improve tests We can simplify the implementation by using defer and by letting the Load() method call Add(0). We can improve tests by making many goroutines updated the atomic int64 value concurrently. * refactor(fsx): can live in the ./internal pkg Let us reduce the amount of nesting. While there, ensure that the package only exports the bare minimum, and improve the documentation of the tests, to ease reading the code. * refactor: move runtimex to ./internal * refactor: move shellx into the ./internal package While there, remove unnecessary dependency between packages. While there, specify in the contributing guidelines that one should use x/sys/execabs instead of os/exec. * refactor: move ooapi into the ./internal pkg * refactor(humanize): move to ./internal and better docs * refactor: move platform to ./internal * refactor(randx): move to ./internal * refactor(multierror): move into the ./internal pkg * refactor(kvstore): all kvstores in ./internal Rather than having part of the kvstore inside ./internal/engine/kvstore and part in ./internal/engine/kvstore.go, let us put every piece of code that is kvstore related into the ./internal/kvstore package. * fix(kvstore): always return ErrNoSuchKey on Get() error It should help to use the kvstore everywhere removing all the copies that are lingering around the tree. * sessionresolver: make KVStore mandatory Simplifies implementation. While there, use the ./internal/kvstore package rather than having our private implementation. * fix(ooapi): use the ./internal/kvstore package * fix(platform): better documentation
2021-06-04 10:34:18 +02:00
## Implementation requirements
- use `./internal/atomicx` rather than `atomic/sync`
- do not use `os/exec`, use `x/sys/execabs` or `./internal/shellx`
refactor: flatten and separate (#353) * refactor(atomicx): move outside the engine package After merging probe-engine into probe-cli, my impression is that we have too much unnecessary nesting of packages in this repository. The idea of this commit and of a bunch of following commits will instead be to reduce the nesting and simplify the structure. While there, improve the documentation. * fix: always use the atomicx package For consistency, never use sync/atomic and always use ./internal/atomicx so we can just grep and make sure we're not risking to crash if we make a subtle mistake on a 32 bit platform. While there, mention in the contributing guidelines that we want to always prefer the ./internal/atomicx package over sync/atomic. * fix(atomicx): remove unnecessary constructor We don't need a constructor here. The default constructed `&Int64{}` instance is already usable and the constructor does not add anything to what we are doing, rather it just creates extra confusion. * cleanup(atomicx): we are not using Float64 Because atomicx.Float64 is unused, we can safely zap it. * cleanup(atomicx): simplify impl and improve tests We can simplify the implementation by using defer and by letting the Load() method call Add(0). We can improve tests by making many goroutines updated the atomic int64 value concurrently. * refactor(fsx): can live in the ./internal pkg Let us reduce the amount of nesting. While there, ensure that the package only exports the bare minimum, and improve the documentation of the tests, to ease reading the code. * refactor: move runtimex to ./internal * refactor: move shellx into the ./internal package While there, remove unnecessary dependency between packages. While there, specify in the contributing guidelines that one should use x/sys/execabs instead of os/exec. * refactor: move ooapi into the ./internal pkg * refactor(humanize): move to ./internal and better docs * refactor: move platform to ./internal * refactor(randx): move to ./internal * refactor(multierror): move into the ./internal pkg * refactor(kvstore): all kvstores in ./internal Rather than having part of the kvstore inside ./internal/engine/kvstore and part in ./internal/engine/kvstore.go, let us put every piece of code that is kvstore related into the ./internal/kvstore package. * fix(kvstore): always return ErrNoSuchKey on Get() error It should help to use the kvstore everywhere removing all the copies that are lingering around the tree. * sessionresolver: make KVStore mandatory Simplifies implementation. While there, use the ./internal/kvstore package rather than having our private implementation. * fix(ooapi): use the ./internal/kvstore package * fix(platform): better documentation
2021-06-04 10:34:18 +02:00
- use `./internal/fsx.OpenFile` when you need to open a file
refactor: flatten and separate (#353) * refactor(atomicx): move outside the engine package After merging probe-engine into probe-cli, my impression is that we have too much unnecessary nesting of packages in this repository. The idea of this commit and of a bunch of following commits will instead be to reduce the nesting and simplify the structure. While there, improve the documentation. * fix: always use the atomicx package For consistency, never use sync/atomic and always use ./internal/atomicx so we can just grep and make sure we're not risking to crash if we make a subtle mistake on a 32 bit platform. While there, mention in the contributing guidelines that we want to always prefer the ./internal/atomicx package over sync/atomic. * fix(atomicx): remove unnecessary constructor We don't need a constructor here. The default constructed `&Int64{}` instance is already usable and the constructor does not add anything to what we are doing, rather it just creates extra confusion. * cleanup(atomicx): we are not using Float64 Because atomicx.Float64 is unused, we can safely zap it. * cleanup(atomicx): simplify impl and improve tests We can simplify the implementation by using defer and by letting the Load() method call Add(0). We can improve tests by making many goroutines updated the atomic int64 value concurrently. * refactor(fsx): can live in the ./internal pkg Let us reduce the amount of nesting. While there, ensure that the package only exports the bare minimum, and improve the documentation of the tests, to ease reading the code. * refactor: move runtimex to ./internal * refactor: move shellx into the ./internal package While there, remove unnecessary dependency between packages. While there, specify in the contributing guidelines that one should use x/sys/execabs instead of os/exec. * refactor: move ooapi into the ./internal pkg * refactor(humanize): move to ./internal and better docs * refactor: move platform to ./internal * refactor(randx): move to ./internal * refactor(multierror): move into the ./internal pkg * refactor(kvstore): all kvstores in ./internal Rather than having part of the kvstore inside ./internal/engine/kvstore and part in ./internal/engine/kvstore.go, let us put every piece of code that is kvstore related into the ./internal/kvstore package. * fix(kvstore): always return ErrNoSuchKey on Get() error It should help to use the kvstore everywhere removing all the copies that are lingering around the tree. * sessionresolver: make KVStore mandatory Simplifies implementation. While there, use the ./internal/kvstore package rather than having our private implementation. * fix(ooapi): use the ./internal/kvstore package * fix(platform): better documentation
2021-06-04 10:34:18 +02:00
- use `./internal/netxlite.ReadAllContext` instead of `io.ReadAll`
and `./internal/netxlite.CopyContext` instead of `io.Copy`
## Code testing requirements
Make sure all tests pass with `go test -race ./...` run from the
top-level directory of this repository.
## Writing a new OONI experiment
When you are implementing a new experiment (aka nettest), make sure
you have read the relevant spec from the [ooni/spec](
https://github.com/ooni/spec) repository. If the spec is missing,
please help the pull request reviewer to create it. If the spec is
not clear, please let us know during the review.
To get a sense of what we expect from an experiment, see the [internal/tutorial](
https://github.com/ooni/probe-cli/tree/master/internal/tutorial) tutorial
## Branching and releasing
The following diagram illustrates the overall branching and releasing
strategy loosely followed by the core team. If you are an external
contributor, you generally only care about the development part, which
is on the left-hand side of the diagram.
![branching and releasing](docs/branching.png)
Development uses the `master` branch. When we need to implement a
feature or fix a bug, we branch off of the `master` branch. We squash
and merge to include a feature or fix branch back into `master`.
We periodically tag `-alpha` releases directly on `master`. The
semantics of such releases is that we reached a point where we have
features we would like to test using the `miniooni` research CLI
client. As part of these releases, we also update dependencies and
embedded assets. This process ensures that we perform better testing
of dependencies and assets as part of development.
The `master` branch and pull requests only run CI lightweight tests
that ensure the code still compiles, has good coverage, and we are
not introducing regressions in terms of the measurement engine.
To draft a release we branch off of `master` and create a `release/x.y`
branch where `x` is the major number and `y` is the minor number. For
release branches, we enable a very comprehensive set of tests that run
automatically with every commit. The purpose of a release branch is to
make sure all checks are green and hotfix bugs that we may discover
as part of more extensively testing a release candidate. Beta and stable
releases should occur on this branch. Subsequent patch releases should
also occur on this branch. We have one such branch for each `x.y`
release. If there are fixes on `master` that we want to backport, we
cherry-pick them into the release branch. Likewise, if we need to
forward port fixes, we cherry-pick them into `master`. When we backport,
the commit message should start with `[backport]`; when we forward
port, the commit message should start with `[forwardport]`.
When we branch off release `x.y` from `master`, we also need to bump
the `alpha` version used by `master`.
In addition we have `PRODUCT-staging` branches for each product. These
branches are described below:
- `miniooni-staging`: branch for building and publishing `miniooni`
binaries. This branch marges from the `master` branch when we tag
alpha releases. We use this strategy because `miniooni` is an alpha client.
We publish the resulting binaries in alpha releases.
- `mobile-staging`: branch for building and publishing iOS and
Android releases of the `pkg/oonimkall` library. This branch
merges from the release branches or the release tags. We use this
strategy because mobile releases are either beta quality or
production quality releases. We publish the resulting binaries
in beta quality or stable releases.
- `oohelperd-staging`: branch for building and publishing `oohelperd`
binaries. This branch merges from the release branches or the
release tags. We use this strategy because `oohelperd` releases
are either beta quality or production quality releases. We
publish the resulting binaries in OONI's internal Debian repository.
- `ooniprobe-staging`: branch for building and publising `ooniprobe`
binaries. This branch merges from the release branches or the
release tags. We use this strategy because `ooniprobe` releases
are either beta quality or production quality releases. We
publish the resulting binaries both in beta quality or stable
releases and in OONI's public Debian repository.
Builds proper happen either in this repository, using GitHub
actions, or using private build machines. Sometimes we need
private build machines to inject specific secrets into the
binary (e.g., we currently use this to inject OONI's Psiphon config).
Additionally, we currently publish as Github Actions artifacts
`miniooni` binaries for selected Linux architectures for
each commit that lands into the `master` branch. You will
find those binaries [in the results of each individual build](
https://github.com/ooni/probe-cli/actions/workflows/miniooni.yml)
until they are automatically removed by GitHub compatibly with
the build assets retention period (should be 90 days).