Guidelines/Contribution process

From Apertis
Jump to: navigation, search

Contents

Contribution process

The process for contributing code to Apertis is the same as for many other open source projects: check out the code, make changes locally, then submit them for review and merging. Apertis uses GitLab.

This page assumes good working knowledge of git; see Version control for specific guidelines.

This is a rough expectation of what the contribution process should look like. It should be followed by all developers, including core and third party contributors.

Branch

Propose changes by pushing a git branch to GitLab.

  1. Check out the git repository:

    git clone https://gitlab.apertis.org/[group name]/[repository name].git

  2. Create a local branch and make code changes in there. The branch should have the following naming convention: wip/$username/$feature or wip/$username/$taskid. This branch should be pushed into the destination repository: if developers lack permissions to do so they can ask to be granted the needed privileges in the Mattermost channel or push to a personal repository instead.
  3. Commit the changes (see the guidelines for making commits), test them and check you are allowed to submit them under the project’s license:

    git commit -i -s

Merge Request

  1. Once the changes are ready to be reviewed, create a merge request on GitLab.
  2. The merge request should have the "Remove source branch when merge request accepted" option enabled.
  3. If changes are not ready and some (strongly encouraged) early feedback is required, the merge request should be marked as Work In Progress (WIP).
  4. Every commit must have an appropriate Signed-off-by: tag in the commit message, see the section below about them.
  5. Add a Fixes: APERTIS-<task_id> tag for each task in the proposed commit messages (as explained in the section Automatically closing tasks) or in the envelope message of the merge request itself in order to link the merge request to one or more tasks in Phabricator.
    • Note: The tag will ensure that Phabricator tasks are kept up-to-date with regard to the status of related merge requests, through the creation of a new comment with the link to the merge request every time a merge request is created/updated/merged. This syntax has been chosen for the tag because it is already supported by gitlab

Review

  1. The repository maintainer or another developer will review the merge request.
  2. Notify maintainers on the Mattermost channel when submitting or updating a merge request.
  3. If changes are needed, a force-push with the changes is required to the submitted branch to update the commits that are part of the merge request (always remember to use --force-with-lease instead of --force).
  4. Reviews should happen on GitLab.
  5. Reviewers can propose concrete code snippets that the submitter can decide to integrate in their patch series or not:
    • Reviewers can use code blocks in comments, and the submitter can copy and paste them in their patches when updating the merge request
    • Reviewers can edit the the submitted branch with the GitLab Web IDE or checking it out manually, stacking new commits on top of the submitted patches: it's up to the submitter to cherry-pick or reject the suggested changes and squash them appropriately with the original commits before the request can be merged
    • If submitters do not agree with the suggested snippets they need to explain why they are rejecting the proposed changes
  6. Questions, request to clarify and any other kind of discussion should be handled via comments on the merge request.
  7. Repeat these review steps until all the changes are accepted.

Merge

  1. Merge request must have no thumbs down to be merged and all the discussions should be resolved.
  2. Merge requests must be approved by at least one reviewer, either by adding a thumb up, or directly merging it.
  3. Once all comments have been handled, the reviewer hits the merge button or otherwise pushes the submitted commits to the appropriate branch. The developer can use the WIP marker to prevent the merge to happen in order to control the moment at which it occurs. In this case, the developer could remove the WIP marker and press the merge button if he has at least one thumb up, provided no additional change occurred.

Extra Actions

  1. If other actions need to be manually taken when commits are landed (for instance, updating the jobs on Jenkins when changing the templates) the developer accepting and merging the changes is responsible for ensuring that those actions are taken, either by doing them themselves or by asking someone else to do that.
  2. If the merged commits need to be backported to other branches through cherry-pick or merges, those should go through merge requests as well: however, only changes related to the backport process are allowed, unrelated changes should be applied with a separate merge request, possibly landing in master first.

Release Commits

  1. Changes to the debian/changelog file of the package should leave the new version marked as UNRELEASED during the review.
  2. Once all the changes have been accepted and merged, a release commit should be made. This release commit can be pushed directly without going through merge requests.
  3. The release commit should only update the debian/changelog file changing the UNRELEASED tag to apertis.
  4. A tag should be generated for this commit and pushed to the repository before pushing the release commit itself.
  5. Once the tag has been pushed, the release commit should be pushed directly to the repository, without any review: the infrastructure will automatically trigger a new package build with the new version as expected.
    • Note: The developer doing the release commit might need special permissions in the repository to push tags and commit the changes directly without any review.

Other important bits

Sign-offs

Like the git project and the Linux kernel, Apertis requires all contributions to be signed off by someone who takes responsibility for the open source licensing of the code being contributed. The aim of this is to create an auditable chain of trust for the licensing of all code in the project.

Each commit which is pushed to git master must have a Signed-off-by line, created by passing the --signoff/-s option to git commit. The line must give the real name of the person taking responsibility for that commit, and indicates that they have agreed to the Developer Certificate of Origin. There may be multiple Signed-off-by lines for a commit, for example, by the developer who wrote the commit and by the maintainer who reviewed and pushed it:

Signed-off-by: Random J Developer <random@developer.example.org>
Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org>

Apertis closely follows the Linux kernel process for sign-offs, which is described in section 11 of the kernel guide to submitting patches.

Automatically closing tasks

You should never close a task manually. To automatically close the task when your changes appear in the packages, add a meta-tag in the git commit message description, pointing to one or more Apertis tasks, each on its own line:

Fixes: APERTIS-<task_id>

When debian/changelog file is generated from the commit messages by gbp dch, that meta-tag is transformed into (Fixes: APERTIS-T12345) appended to the changelog message. When the package is built and uploaded, the task will be automatically closed.

By default, only the first line of the commit message is used for the changelog entry.

For example, for a commit message like this:

Update website link to demo images

The website has new path for demo images.

Fixes: APERTIS-T12345
Thanks: John Smith for pointing out the issue

Signed-off-by: Random J Developer <random@developer.example.org>

will be converted into the following changelog entry:

* Update website link to demo images.
  Thanks to John Smith for pointing out the issue (Fixes: APERTIS-T12345)

Privileged processes

Pushing commits to gitlab.apertis.org requires commit rights which are only granted to trusted contributors. All commits must have a Signed-off-by line assigning responsibility for their open source licensing.

Packaging up and releasing new versions of Apertis modules as Debian packages requires packaging rights on build.collabora.co.uk (OBS), which are issued separately from commit rights.

Submitting automated test runs on lava.collabora.co.uk requires CI rights, which are granted similarly to packaging rights. However, CI results may be viewed read-only by anyone.

Getting commit rights

Commit rights (to allow direct pushes to git, and potentially access to the package building system, build.collabora.co.uk) may be granted to trusted third party contributors if they regularly contribute to Apertis, with high quality contributions at the discretion of current Apertis maintainers. If you are interested in requesting commit rights, you should talk to the Apertis maintainer or patch reviewer with whom you work most often.

API review process

Before a public API may be marked as stable and suitable for use by third-party apps, it must be reviewed. The process for this is:

  1. Some or all of the APIs of a component are selected for review. For example, it might be decided to review the C APIs for a component, but not the D-Bus APIs which it uses internally to communicate with a background service.
  2. The component authors produce:
    • A description of the goals of the component and the APIs under review. These should be high-level descriptions, covering the use cases for that component and its APIs, goals of the implementation, and specific non-goals. Where possible, this should refer back to a design document. These descriptions should not be simply a copy of the API documentation describing how the goals are implemented.
    • A list of required features and APIs which must pass review before the API may be marked as stable.
    • A list of other components which depend on the APIs under review, and a description of how those other components should interact with the APIs under review. This ensures the final APIs are still usable as intended from other components, and is another way of describing the use cases of the API.
    • An explicit description of where privilege boundaries are required in the system, and whether any of the APIs cross those boundaries (for example, if they implement functionality which should not be callable without elevated privileges).
    • A description of how the API may need to be extended. For example, should it be extensible by the component maintainers, by third-party apps, or not at all?
  3. The reviewers examine the APIs and provided documentation, and make recommendations for the component as a whole, its interactions with other components, and specific recommendations for each API under review (for example, ‘keep this API unchanged’, ‘remove this API’, ‘keep it with the following changes’, etc.). See the API design guidelines for some principles that the reviewers might apply.
  4. The feedback is discussed, potentially iterated, and changes are made to the public API accordingly.
  5. The changes are re-reviewed, and the process iterates until the reviewers are happy.
  6. The APIs are marked as public and stable, and their API documentation is updated.

The main goal of the review process is to ensure that the stable APIs, which have to be supported for a long time in the future, are a good match for all current and anticipated use cases, and follow good system architecture practices. A large part of this effort is in ensuring that all use cases for the APIs are known and documented. A non-goal of the review process is to look at each API individually and mechanically run through some ‘API review checklist’ — this can easily miss mismatches between the API and the component’s use cases; architectural issues which are important to catch.

Components’ APIs should not be marked as stable until they have been reviewed.

Personal tools
Namespaces

Variants
Actions
Navigation
Tools