diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e8dc3af9..252c878d7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,41 +6,32 @@ We welcome code contributions from the community and are very happy to receive f Issue Tracker ---- -This project uses the Github issue tracker. Please report bugs, issues and feature requests there or email us at otaconnect.support@here.com. +This project uses the Github issue tracker. Please use that for bug reports and to discuss new features prior to implementation. Code quality and style ---- All code should be developed according to the [Google C++ style guide](https://google.github.io/styleguide/cppguide.html). In addition, the code should conform to the following guidelines: -* All code should be covered by tests. - - Ideally, all tests should be automated and run in CI. - - If automated testing is not possible, manual test cases should be described. - - Tests cases should at least exercise all documented requirements. -* It must be easy for a new developer to run the test suite based on the information in the [readme](README.adoc). -* All code must pass formatting and static linting tests. -* Only working code that passes all tests in CI will be merged into the master branch. - - The master branch should always be in a deployable state. - - Functionally incomplete code is not necessarily undeployable. -* Pull requests should only contain a single feature, fix, or refactoring. - - Developers should not create large, month-long, multiple-feature branches. - - The larger the PR, the harder it is to review, the more likely it will need to be rebased, and the harder it is to rebase it. - - If a PR changes thousands of lines of code, please consider splitting it into smaller PRs. - - Developers should aim to have their code merged at least once every week. - - Multiple small fixes or refactorings can be grouped together in one PR, but each independent fix or refactoring should be a unique commit. +* The best way to ensure new features don't get broken is to test them in Aktualizr's CI. +* Formatting, static analysis and green tests are enforced by CI. +* The master branch should always be in a deployable state. Functionally incomplete code is allowed in master as long as it doesn't break other things, and is better than long-lived feature branches. +* Aim to keep PRs small. * Make separate commits for logically separate changes within a PR. - - Ideally, each commit should be buildable and should pass the tests (to simplify git bisect). + - Each commit should be buildable and should pass the tests (to simplify git bisect). - The short description (first line of the commit text) should not exceed 72 chars. The rest of the text (if applicable) should be separated by an empty line. -* All code must be reviewed before it is merged to the master branch. - - The reviewer can not be the code author. - - All the reviewer's concerns must be resolved to reviewer's satisfaction. - - Reviews should enforce the coding guidelines for the project. * Bugs should be reproduced with a failing test case before they are resolved. - - Reviewers of code including bug fixes should check that a corresponding test has been included with the fix. -* Code that is unfinished or that requires further review should be indicated as such with a `TODO` comment. - - If appropriate, this comment should include a reference to a Jira ticket that describes the work to be done in detail. - - Since external contributors do not have access to our Jira, the comment must at least briefly describe the work to be done. -* New features, bug fixes, and removed functionality should be documented in the [changelog](CHANGELOG.md). + +Reviewing Changes +----------------- +Aktualizr is developed by and for a group of developers that span different organizations with different goals and needs. We want to make rapid progress--upstream first!--while not breaking each other. To balance these we: + +* Require all code changes to have a +1 review by someone who isn't the author. +* Prefer the reviewer to have a different organizational affiliation to the author +* After 3 business days, a +1 from anyone except the author is enough +* In all cases the CI should be green before merging +* Reviews within the same organization are encouraged. This might mean a PR gets two reviews. That is good. + Making a Pull Request ---- @@ -52,11 +43,8 @@ When your feature is ready, push the branch and make a pull request. We will rev Continuous Integration (CI) ---- -We currently have two CI servers: Travis CI and gitlab. Travis CI is usually slower and flakier, and we don't run the tests that require provisioning credentials on it, but it is publicly accessible. Gitlab is more powerful and is the source of truth, but it is inaccessible to external contributors. Normally, we expect PRs to pass CI on both CI servers, but if Travis CI is particularly unreliable, we sometimes make exceptions and ignore it. - -PRs from external contributors will not automatically trigger CI on gitlab. If the PR is small and we believe that passing Travis CI is good enough, we will merge it if that succeeds. Otherwise, we can trigger gitlab to run CI on your PR by manually pushing the branch to gitlab. +We use GitHub's CI. The configuration is in `.github/workflows`. Changes to that go through the same review process as other code changes. -PRs that only affect documentation do not strictly need to pass CI. As such, gitlab does not run CI on branches that start with "docs/". Please do not make code changes in a branch with that prefix. Developer Certificate of Origin (DCO) ----