ooni-probe-cli/CONTRIBUTING.md
Simone Basso fdbf871103
[forwardport] ci/cd: publish binaries onto a release when we create a tag (#609) (#611)
This diff forwardports 856e436e20d511a4f0d618546da7921fa9f8c5f6 to the master branch

Original commit message:

- - -

This pull request changes `mk` and github workflows to build and publish binaries on tag. We also update the documentation to explain this new branching model. Basically, we have release branches where we produce binary packages and we add extra code, on tag, to publish such packages inside a release.

We discussed removing most secrets from builds in this repository and having a different tool/repository that takes in input also secrets for doing follow-up actions after publishing. As a consequence, this pull request also removes all pieces of code that require secrets. The next step is to reinstate this code in this new repository/tool.

The existing code in `mk` also implemented caching. This feature was useful when doing local builds because it reduced the time required to obtain binary releases. With builds running as part of GitHub actions, we don't need caching because we spawn parallel machines to build binaries. Therefore, let us also remove caching, which makes the code simpler. (Caching in itself is hard and in https://github.com/ooni/probe/issues/1875 I noted that, for example, caching of the `ooni/go` repository was leading to some unwanted behaviour when changing the branch. Without caching, this behaviour is gone and we always generally use fresh information to produce builds.) Of course, this means that local builds are now slower, but I do not think this is a problem _because_ we want to use GitHub actions for building in the common case.

Reference issues: https://github.com/ooni/probe/issues/1879 and https://github.com/ooni/probe/issues/1875.

The final aspect to mention to conclude this description is an implementation one:

```
          gh release create -p $tag --target $GITHUB_SHA || true
```

The code above uses `|| true` because there could already be a release. So, basically, it means that, if a release does not already exist, then we're going to create one. Otherwise, it does not matter because there's already a release.
2021-11-23 15:56:25 +01:00

5.7 KiB

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

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.

Implementation requirements

  • use ./internal/atomicx rather than atomic/sync

  • do not use os/exec, use x/sys/execabs or ./internal/shellx

  • use ./internal/fsx.OpenFile when you need to open a file

  • 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 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 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

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.

We build binary packages for each tagged release. We will use external tools for publishing binaries to our Debian repository, Maven Central, etc.