Keyboard shortcuts

Press ← or → to navigate between chapters

Press S or / to search in the book

Press ? to show this help

Press Esc to hide this help

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 Tarook GitLab group.

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.

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.

  1. The Developer now takes the steps necessary to resolve the issue.

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

  3. When development is done and the CI passes, the Developer sets the Review needed label.

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

  5. The Reviewer picks a Merge request with the Review needed 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.

  6. Once the Reviewer is happy the Reviewer adds their approval to the Merge request.

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

TAROOK specialities

As the CI of Tarook 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 Testing needed 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 Testing needed label, independent signalling for Review done and Testing done is needed (which GitLab-level approvals can't give). Hence, after review and testing, the Review done and Testing done labels should be set, respectively (and the corresponding "needed" 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.