Contribution workflow#

This document describes the steps to follow when contributing to the SCL project and its siblings. It applies to all contributions, such as new features, bug fixes, documentation, and test enhancements. For our general ground rules, please see the "General Development Guidelines" section in the introduction.

Stage 1: The Issue#

We shall always open an Issue (in the relevant GitLab project) for documenting each task. If there is no Issue, there is no task.

• Use our Issue template. The Issue shall explain:
  • What is the motivation or problem?
  • Where/when/how does it happen? (In case of a bug, this includes steps to reproduce it.)
  • Why is this an issue - why should we address it?
  • What are relevant resources that we already know of? What Issues, code, Merge Requests, or documentation parts are related?
  • What is the approach to address the Issue? Document any plan(s) already present.
• The Issue shall be properly labeled (identifying the components), associated with the relevant Work Package / Epic / Milestone, and assigned (who is working on it right now?). If you cannot do it yourself, ask a project manager.
• Do not start working on a new Issue without synchronizing with the team. You can make proposals, but at least get an "ACK" in the chat before starting to work on it.

Stage 2: The Design#

Before starting to actually implement the solution to an Issue, you shall make a plan on how to address it. The resulting plan(s) shall always be documented in the Issue.

• If you are not sure how to do it, consult the team or set up a meeting for a conceptual discussion.
• If you feel incapable to do it, ask a project manager to re-assign it.
• Make sure the Issue is assigned to the one working on it and has the label "Status: WIP".
• Provide a valid reasoning for your decisions that can be understood by the team.
• In case of architectural decisions (or impact on earlier decisions / architecture), the new high level picture should be supported with a new or updated figure (to be added to our architecture documentation in the Merge Request).
• Ask for feedback if you are not sure if this is the one and only right way to go.
• If you have questions, ask them via shared channels so that you will get a timely answer and make sure the answer is documented for the future. If you had chat discussions on a specific Issue, the simplest way to document them is to quote a copy of the chat history in the Issue.

Of course, you may always gather additional information in the later process (e.g., when coding) and so plans may change. In this case, the documented plan(s) should be updated and this should be communicated to the team (easiest via a comment in the Issue).

Stage 3: The Coding#

When you are done with the initial design phase and you are sure about the direction, you can start with the implementation.

Code#

• Your code should be as simple and self-documenting as possible.
• Comment non-obvious things and document all interfaces to other building blocks. In this context, adhere to our Code Documentation Guideline.
• Do not try to be elitist and leverage all esoteric features of the language and tools, even if it is sometimes tempting. Boring code with slight repetitions is often better than the shortest (and more often than not) most convoluted option.
• Aim for homogeneous formatting. Adhere to our Code Formatting Guideline.
• Respect the module structure and directory layout. When changes to it are necessary, ensure that they have been properly discussed during the design phase. There should most probably be an updated architecture diagram somewhere in this case.

Documentation#

• In the course of implementing each change, make sure that the project documentation is up-to-date.
• The current state of the code on the main branch should always match the documentation beneath it.
• Ask yourself if the developer, operator, and user documentation relevant for the functionality you are touching is sufficient and understandable.
• Concepts derived in the design phase shall be moved to the documentation when implemented (including relevant updates if anything changed in the process).
• We should not post-document but document along with each feature (i.e., have the docs contained in the same Merge Request).

Tests#

Tests ensure not only that you capture most current bugs that you can think of, but that you also freeze the intended and expected behavior and get notified if it changes unexpectedly - which may mean that the initial assumptions do not hold anymore and someone should take a look at the potential impact. If someone breaks something by a seemingly unrelated change and the tests stay green, it is your (i.e., you who did not write the test for it) responsibility to get it fixed.

Thus, add tests for all new non-trivial functionality. You can choose as implementer which way of testing is the most appropriate, e.g., unit tests, single-component integration-tests, full on-device hardware-software tests, ... In most cases, the answer will be a combination.

Commits#

• Before committing anything, create a new branch from the current main branch. Format branch names like this: <issue ID>-this-is-a-short-description, whereas issue ID is the number of the Issue. If there is no Issue, go back to Stage 1.
• Keep feature branches atomic if possible (one branch and Merge Request per feature / issue)
• In general, commits should be atomic (change one thing, but all of it). Git's blame, log, revert, and bisect are powerful tools and you should make it as easy as possible to use them with your commits even in the far future.
  • A commit should transform the code from one good state (CI passes) to another good state, just like DB transactions: If we later identify a problem with the software, it would be perfect if we can revert a single commit to roll a change back to a previous good state.
  • WIP state should be avoided (e.g. several commits touching the same small piece of code to gradually achieve a single goal (first commit: "handle corner case X, mark other as TODO", second commit: "handle corner case Y", ... ) should be combined in a single commit). This also makes it easier to review commit by commit.
  • Avoid "shotgun" commits: a commit should be about a single topic. Several unrelated changes in a commit (move files + refactor some logic + add feature X) makes it harder to revert it. It is better to split them up in 3 commits in this case. Note that the key point here is unrelated: adding a feature, tests for it, and corresponding documentation in a single commit is totally fine.
  • Also note that achieving the desired history in most cases requires rebasing. It is not uncommon to have some WIP / messy commits during development of the feature ("make it work" phase) and then reorganize the changes into new tidy commits ("make it right") before handing in the Merge Request.
  • Another good resource on that: https://www.kernel.org/doc/html/v4.16/process/howto.html#break-up-your-changes
• Squashing vs. small individual changes: Occasionally, the question arises whether or not to squash together changes related to an Issue or keep the individual commits (that may amount to a two-to-three-digit number). There are several philosophies to approach this. As the nature of changes and the corresponding benefits and/or drawbacks resulting from squashing are often very different, we intentionally do not mandate a specific model, but rather give some hints for a case-by-case decision:
  • The general goal is always to keep the development history traceable and comprehensible. E.g., when running git blame on a specific line corresponding to a bug, as much information as possible should be extractable from the commit message on why this line has been written the way it was.
  • A second goal is debuggability: Sometimes one will need to track down a bug through a chain of commits, e.g., using git bisect. In this regard, the state at every commit should ideally be executable and pass basic tests.
  • A goal with lower priority and mostly relevant for smaller patches and bug fixes is to make the change easily revertable (via git revert).
  • Do not squash unrelated changes together. If there are 69 commits with 68 pertaining to a specific big change and one being another small change in the context of the same Issue and you decide for squashing, the result should be two commits (68 commits squashed and the remaining commit staying separate).
• Write a good commit message.
  • See: https://cbea.ms/git-commit/
  • Write about the "What", "Why", the impact (high-level perspective) for the project, and important design aspects such as tradeoffs taken.
  • Reference related resources (e.g., documentation, issues, ...).
  • Voice: We strongly prefer commit messages in imperative voice: This works well with the following mental model: "When you add / cherrypick this commit, it will $commit_title_line (to the code base). Or do you want to revert $commit_title_line?
  • Title: Use a short, descriptive title and sentence case without a dot at the end. If possible, identify the high-level component (the crate / "doc" / "nix") at the beginning, in lowercase, potentially abbreviated (like "l3-ctrl"), and followed by a colon and space.

Stage 4: The Review#

Creating the Merge Request#

• Use our Merge Request template, answer all questions, and fill out all gaps.
• Reference the addressed and related Issue(s) in the description, as well as related other Merge Requests. If there is no Issue, go back to Stage 1.
• Titles of merge requests should be in imperative mood like commit headings.
• Keep Merge Requests in Draft state as long as you work on them or something else is blocking them.
• Always assign all Merge Requests to somebody (the one who should work/act on it next).

The review process for MR submitters#

• Request a review of the Merge Request when all open points have been addressed, the tests work, you think that it meets our quality standards, and you are in general happy with the result. If the Pipeline is red, it is your responsibility and not that of the reviewers to fix it.
• If you noticed something that you want to change immediately, just set the reviewer to "Unassigned".
• Set the corresponding Issue(s) label to "Status: Review"
• If you receive feedback...
  • If you understand the concern and agree with it: come up with a good enough change.
  • If you don't understand the concern: ask for clarification.
  • If you don't agree with it: state and elaborate your opinion, discuss with the reviewer(s). If there is continuous disagreement or there seem to be two different approaches with pros and cons, ask for a dedicated meeting or discuss the topic in the next Weekly.
• In general, don't close discussion threads opened by others on your own. The person asking should decide on whether their question has been resolved and act on it. Common exceptions:
  • A suggestion (a patch) or trivial proposal resolving the concern was applied or
  • the concern was moved into a dedicated issue.

The review process for MR reviewers#

• Keep quality standards in mind while reviewing changes.
• If in doubt, ask why a certain design approach was implemented to understand its trade-offs.
• Justify criticism and elaborate the effect of non-trivial bugs if it might not be obvious to others.
• Try to keep feedback actionable, but do not overdo it (the assignee should do the heavy lifting / thinking).
• Mark any minor comments that don't block the MR with Nit: (e.g., to voice your very subjective opinions).
• Submit feedback in batches / bulks / "reviews" to keep email noise low.