2021-02-03 14:42:51 +01:00
|
|
|
# Contributing to ooni/probe-cli
|
2021-02-02 12:05:47 +01:00
|
|
|
|
|
|
|
|
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.
|
|
|
|
|
|
2021-08-19 18:55:49 +02:00
|
|
|
## OONI Software Development Guidelines
|
2021-02-02 12:05:47 +01:00
|
|
|
|
2021-08-19 18:55:49 +02:00
|
|
|
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.
|
2021-02-02 12:05:47 +01:00
|
|
|
|
2021-08-19 18:55:49 +02:00
|
|
|
## Opening issues
|
2021-02-03 14:42:51 +01:00
|
|
|
|
2021-08-19 18:55:49 +02:00
|
|
|
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).
|
2021-02-03 14:42:51 +01:00
|
|
|
|
2021-02-02 12:05:47 +01:00
|
|
|
## 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
|
2021-08-19 18:55:49 +02:00
|
|
|
using `go test -short`. We prefer internal testing to external
|
2021-02-02 12:05:47 +01:00
|
|
|
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
|
|
|
|
|
|
2021-06-04 11:39:00 +02:00
|
|
|
- use `./internal/atomicx` rather than `atomic/sync`
|
|
|
|
|
|
2021-06-04 14:02:18 +02:00
|
|
|
- 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
|
|
|
|
2021-06-04 14:02: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
|
|
|
|
2021-09-28 12:42:01 +02:00
|
|
|
- use `./internal/netxlite.ReadAllContext` instead of `io.ReadAll`
|
|
|
|
|
and `./internal/netxlite.CopyContext` instead of `io.Copy`
|
2021-06-15 11:57:40 +02:00
|
|
|
|
2021-02-02 12:05:47 +01:00
|
|
|
## 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.
|
|
|
|
|
|
2021-10-22 15:41:53 +02:00
|
|
|
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.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
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 branch off release `x.y` from `master`, we also need to bump
|
|
|
|
|
the `alpha` version used by `master`.
|
|
|
|
|
|
|
|
|
|
None of the branches described so far automatically publishes any
|
|
|
|
|
binary package. To publish binary packages we use the `PRODUCT-staging`
|
|
|
|
|
branches (e.g., `mobile-staging`). To trigger these builds, we merge
|
|
|
|
|
from a release branch into the desired `PRODUCT-staging` branch. This
|
|
|
|
|
design ensures that we only publish binary packages when we want to
|
|
|
|
|
do so. Because we merge from a release branch, we are not pushing alpha
|
|
|
|
|
code to binary releases. (The only exception to this rule is `miniooni`,
|
|
|
|
|
treated differently because it's an `alpha` client.)
|
2021-02-02 12:05:47 +01:00
|
|
|
|