Development Process

This chapter describes the steps for contributors and developers to submit code and documentation, as well as feature requests or bug reports to the project.

Please refer to the subsections for details.

Bug Reports

This still needs to be defined properly.

Feature Requests

Scope

This document applies to all repositories in the Yaook GitLab namespace.

Conventions used in this Document

The keywords "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119.

Workflow

Feature requests, also called Feature Proposals, follow a defined workflow. This workflow may seem cumbersome, but the intent is to avoid waste of time caused by solutions which do not address non-obvious constraints or by work invested by different teams into the same solution without synchronization.

The goal of the process is to have, before implementation begins, a rough specification towards which developers can work. By having the specification created and reviewed collaboratively, the chances for non-obvious constraints causing issues are reduced. This is especially helpful for newcomers to the project, who cannot be expected to know all constraints of the project.

This process is OPTIONAL but highly RECOMMENDED for anything which goes beyond simple fixes. Otherwise, contributors risk that their contribution is rejected because it does not fit constraints the contributors were not aware of.

Life cycle of a Feature Proposal

Creation of an Issue on GitLab

First, an issue MUST be filed on GitLab. This issue SHOULD use the following template and MUST be labeled as ~"workflow:discussion":


/label ~"workflow::discussion"
/assign me

## Summary

<!-- Describe the proposed feature in a few sentences; the more detail, the better, but generally one or two sentences are sufficient if you provide a well laid-out use case -->

### Use Cases

<!-- At least one specific use case needs to be provided. This should be described in as much detail as possible, making the outer constraints clear, so that others know where you're coming from -->

## Proposal

<!-- If available, provide a proposal for an implementation. This should not be very detailed and just a rough sketch. -->

## Specification

The keywords "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this section are to be interpreted as described in [RFC 2119](https://datatracker.ietf.org/doc/html/rfc2119).

<!-- OPTIONAL: Write a proposed specification either as full text or a bullet points. -->

Notes:

The contents of the "Specification" section are optional, we'll get to that later.
You may not have the necessary permissions to assign labels right away; someone else will do it for you then.
The request should be filed in the project it refers to. If your request spans multiple projects, try to find a place where it fits best. Worst case, someone will move it.

Discussion of the proposal and specifying it

The proposal will now be discussed by other contributors. This process may raise issues with the proposal itself, voice support or extend the proposal in order to fit the project better.

When the discussion is over, a specification in the form of either text or bullet points MUST be written and injected into the issue description by either the author of the proposal or by a project maintainer.

The specification MUST be reviewed by a maintainer. Once the specification has passed review, the ~"workflow::specified" label MUST be on the issue.

If the feature proposal gets rejected, the workflow label MUST be retained as-is and the issue SHOULD be closed, unless further discussion is expected to happen in the near future. Feature Proposals MAY be reopened by entities with a concrete proposal to fix the reasons for rejection.
Implementation

A contributor may at any time assign themselves to the feature proposal (or get assigned by a project maintainer on request) to signal that work will start. In that case, the issue is MUST be labelled with the ~"workflow::implementing" label, even if no merge request is created right away.

For the main development process, please see the Developing document.

If during the development process the contributor cannot continue working on it, the labelling MUST be changed:
- if external factors (upstream dependencies, other issues) block the progression of the implementation, the label MUST be changed to ~"workflow::blocked".
- if the contributor cannot spend more time on the issue (e.g. it got deprioritised), the label MUST be changed to ~"workflow::specified" in order to signal that others may take up the work.
Notes:
- There is not necessarily a 1:1 mapping between Feature Proposals and merge requests. Especially for complex features, it might make sense to have multiple smaller MRs to achieve the goal over one gigantonormeous MR.
Finalization

Once the specification has been implemented (all corresponding MRs merged), a project maintainer MUST close the issue to signal its completion. The workflow label SHOULD be retained to see at a first glance whether an issue was (supposedly) completed or not.

Developing

This section describes the process of fixing a bug or implementing a feature, i.e. the actual process of writing the code and getting it into the project.

Scope

This document applies to all repositories in the Yaook GitLab namespace.

Conventions used in this Document

Dramatis personae

Reporter: One or more people who discover an Issue within the product (may be a missing feature, a bug, a missing enhancement or similar)
Developer: One or more people who work on resolving Issues within the product.
Reviewer: A person who reviews the work done by a Developer.

Notes

While Developers and Reviewers generally come from the same pool of people, for a specific Merge request, the Developers and Reviewers SHOULD NOT overlap and MUST NOT be the same set of persons (this is ultimately enforced by GitLab approval rules).
For a specific Issue and associated Merge request, the Reporter and Developer MAY overlap and MAY also be the same person. In that case, filing an Issue is still RECOMMENDED.

Workflow

After taking ownership of an issue by assigning themselves and setting the ~"workflow::implementing" label, a Developer iterates with the following steps until the issue is resolved or they cannot continue work for whatever reason.

See Feature Requests and Bug Reports for the lifecycle of those types of tickets as well as the other process involved around them.

Note: This description mostly assumes a singular merge request per issue. However, it MAY, and for complex issues even SHOULD, be iterated over multiple Merge Requests. This is beneficial for the Reviewer because the changes are more narrowly scoped and for the Developer because they get earlier feedback and faster review.

The Developer now takes the steps necessary to resolve the issue.
The Developer creates a Merge request with changes working toward the resolution of the issue. The Merge request MUST be assigned to the Developer and SHOULD NOT have any labels; labels which have been transferred from the corresponding issue SHOULD be removed. It MAY be marked as Draft if the Developer still needs to do considerable work (e.g. when they expect the CI to fail):
1. If an Issue exists, the Merge request MUST refer to the Issue. If the MR address the issue completely, the reference MUST be Fixes #xyz or Closes #xyz. If the MR does not address the issue completely, the reference SHOULD be In context of #xyz.
At least one of the commits in the MR MUST refer to the issue. If only a single commit is part of the MR, it MUST include the same reference the merge request description already has (i.e. "fixes", "closes" or "in context of").
1. If no Issue exists, the Merge request MUST contain the entire information which would normally be included in the issue, including a complete bug description or a use case for feature requests.
When development is done and the CI passes, the Developer sets the Needs review label.
Reviews and Testing can be done by anybody (however, ultimate approval is required according to the approval rules of the respective repository). This is especially for filling waiting hours or similar tasks. Nevertheless, especially in small teams, scouting for open reviews at least once a day is RECOMMENDED. Additionally the project or team leader will enforce the work on specific issues as required. The Developer MAY also poke specific persons or the IRC channel to review work which is urgent.
The Reviewer picks a Merge request with the Needs review label. They remove the label and add themselves to the reviewers to take review ownership. They are now responsible for moving the Merge request forward.

The Reviewer and the Developer work together to improve and document the code as necessary in order to achieve the most sustainable and maintainable result. Please follow the Review Guide.

Any feedback is raised as Discussions on the code or the Merge request itself by the Reviewer. Discussions SHOULD NOT be resolved by the Developer, but only by the Reviewer after verifying that they have been addressed accordingly. An exception is made for informational statements (e.g. "you could also write it this way [but you do not have to for review to pass]"), where the Developer can mark them as resolved to politely decline the suggestion.

Any changes to the code are again to be reconsidered by the same Reviewer.
Once the Reviewer is happy the Reviewer adds their approval to the Merge request.
Given permissions and a green CI, the Reviewer may now press the Merge button. Otherwise, they assign someone with permissions, possibly after out-of-band discussion.

Notes

Merge requests MUST be set to "Delete source branch"
Merge requests MUST NOT be squashed
Merge requests SHOULD be merged with Merge commit, but MAY be merged fast-forward

yaook/k8s specialities

As the CI of yaook/k8s is known to be non-exhaustive, some changes may need extra testing. In such a case, the following extra rules apply:

The Developer adds the Needs testing to indicate that this MR needs extra work before adding Needs review.
The Developer and the Reviewer agree on when a good time for testing is: depending on the changes, it may be sensible to test before doing an extensive review, in other cases the opposite may be true.
For MRs which have had the Needs testing label, independent signalling for Reviewed and Tested is needed (which GitLab-level approvals can't give). Hence, after review and testing, the Reviewed and Tested labels should be set, respectively (and the corresponding "needs" labels removed).
Once testing and review has completed, the MR is approved and follows the usual steps from there on.

Development Notes.

Adhere to the style guide of the repository (typically, there will be CI jobs enforcing this)
When applying style fixes to existing code, put them in a separate commit.
When applying style fixes to code you wrote, fixup them into the commit which introduced the code.
Use git fetch && git rebase -i --rebase-merges origin/master to clean the history up before sending the MR to a review.
- Adhere to The Guide for Writing Good Git Commits
- remove typo fixes (see the fixup rebase instruction), squash commits to follow the above guidelines and reword commits to make their purpose clear.
Ensure that the MR description shows:
- What the MR attempts to achieve (this can be delegated by linking to an issue, if and only if the MR also Fixes #... that issue)
- Why it is needed (link to the issues, including a Fixes #... if appropriate)
- Which MRs does it depend on (if any)
- Optionally: which MRs do depend on this one
Ensure that the CI is green or that there is a good reason, unrelated to your code, for the CI to be red. If so, make sure to explain a red CI in a comment or in the MR description.
If the repository you are working on has tests, ensure that you add tests for the features you introduce.
If the repository you are working on does not have tests, consider adding tests for the features you introduce.
If the MR depends on other MRs which are not yet ready, remove the WIP flag but do not assign a Needs review label yet. The MR should stay assigned to the Developer.
If others work on the same branch as you, make sure to coordinate regarding pushes and rebases.
Avoid uncoordinated rebases after a Reviewer assigned themselves. Re-take ownership before rebasing and inform and coordinate with the Reviewers before force-pushing.

Review & Testing

Reviewer and Developer should coordinate regarding rebases.
The Reviewer adds discussions (not comments) to the MR, the Developer fixes them and the Reviewer marks them as resolved once the fix is confirmed.
If larger changes are needed, the Reviewer should assign the MR back to the Developer and add the WIP flag. Once the changes are done, the MR is handed back and the WIP flag is removed.
The Reviewer may delegate sub-tasks. In that case, the MR should be re-assigned to the person who is supposed to handle the sub-task, and a comment addressed to that person should be added which explains what needs to be done.
Adhere to the Review Guide

Commit guideline: When to commit?

It is RECOMMENDED to follow the notes in this section, but not REQUIRED, if the requirements implied in the Review Guide are fulfilled.

This section has been produced in collaboration with members of the PowerDNS community IRC and Prosody IM community.

During development

Commit early, commit often
- Think of a git commit as your extended Undo-history. You can go back to any committed state at any time.
- It avoids forgetting to commit when you switch subtopics of a task, where things should be in separate commits, making it harder to do a correct cut.
- The commit messages don't need to be pretty here -- likely you'll squash much of it away anyway.
- Side-note: push often; it is a nice decentralized backup.
If making automated mass changes:
- Make them in a separate commit, a separate MR most likely even
- Document how you did them so that others who have to rebase have less pain

Before review

Think about all your changes and try to batch them into logical units.
In the end, each commit should cover as little scope as possible and as much scope as necessary.
One way to look at it is: there should only ever be a single reason to revert a commit.
- This boils down to: when git bisect ever points at your commit for being the cause of trouble, nobody will want to wade through hundreds of lines of change to figure out what exactly goes wrong.
- Though a commit which adds a new feature may generally be large.
Another way to look at it is: Each commit should individually pass the test suite.
- NB: Tests for a feature belong into the same commit as the feature itself. Unless you're specifically adding tests for existing stuff, no commit should only add tests.
Yet another way: Each commit should be individually reviewable.
- If a MR gets large, it helps the reviewer to go step by step through the commit history of the MR.
- When looking at a git log / git bisect output, one does not want to have to look into context besides the commit message and the diff itself.
If you iterated through designs of a feature, ideally this is reflected in the commit history.
- This gives the reviewer additional context on your design decisions.
- It also allows going back to an earlier design more easily.

When applying review feedback

Use git commit --fixup and git rebase -i --autosquash origin/devel so that the state the reviewer looks at next is correct. Do not staple your fixes at the head of the history.

Review Guide

Scope

This document applies to all repositories in the Yaook GitLab namespace.

Goals of the Review

Improve code quality and maintainability
Spread knowledge about how the system works between code authors and maintainers
Catch errors which cannot be caught by automation early on

Non-goals of the Review

Waste of time
Substitute for unit tests

Dramatis personae

Reporter: One or more people who discover an Issue within the product (may be a missing feature, a bug, a missing enhancement or similar)
Developer: One or more people who work on resolving Issues within the product.
Reviewer: A person who reviews the work done by a Developer.

How to Review

During review, you need to check for the following things:

Is the Merge Request description and title good?

The title should explain what has changed and the body should completely explain why it changed. During the review, no information from the commit messages should be necessary to understand what is going on.

The title MUST NOT match /Resolve ".*"/.
Is the pipeline green?

If the pipeline is not green and the Developer has no convincing argument why you should still review the code, stop here and hand the MR back to the Developer for fixing. Don’t forget to tell them why.
Is the code readable, formatting wise?

Even though we use an automated style checker, some things cannot be enforced by that and there will always be judgement-call edge cases. Ensure that all code is reasonably formatted and can be read.
Is the code understandable, content wise?

A review should not take tremendous amounts of time per line. The code needs to be understandable, even for people not intimately familiar with the system it belongs to. If it is not clear why the code operates or how the code operates, it needs to be explained by the author – preferably in a comment, but in some cases in the commit messages, too.
Have tests been added?

All functions should have unit tests completely covering them (use the coverage markers displayed by gitlab as a first line of defense here). New features in operators should come with API-level semi-integration tests. If possible, new services should be smoke-tested in the integration test in the CI.
Are there any logic errors?

That one speaks for itself. With the information from the previous points you should be able to spot any obvious logic errors.
Have the docs been updated?
- If the code introduces a new operator or dependency, that must be added to the dev start. It must also be added to the docs.
- If the code introduces a new concept, a piece of documentation must be added.
- Environment variables, Kubernetes labels, annotations and taints need to be documented.
- Classes and non-private functions and methods need docstrings and should be linked from the docs.
Are the commit messages good?

A good commit message consists of a subject line summarizing the change followed by one or more paragraphs which explain why the change was necessary. It should contain a link to an issue which has more details and gives more context. They must contain all information given in the MR description.

The guide "How to Write a Git Commit Message" should be followed.

Add comments for anything wrong. If you think that some issues you found will cause changes invalidating a later step of the review, you may point that out and stop the review there to avoid doing work twice.

Hand the MR back to the author and let them apply the fixes. Once the MR satisfies you, add a comment matching the following template, crossing off everything you validated and adding a note explaining why you did not check a specific thing if you skipped it:


- [ ] Pipeline green
- [ ] Code is readable
- [ ] Code is understandable
- [ ] Tests have been added and cover the added functionality as is reasonable
- [ ] Docs have been updated
- [ ] Commit messages look good
- [ ] **LGTM**

You may then approve the MR. If sufficiently privileged, you can hit the Merge button, too.

Note: The comment and approval will appear in the MR history, which allows someone who looks at it later to validate that no changes have been made after the review stage has been completed.

Yaook Development Process Documentation