Issue and merge request workflows

Drew Leske

We create issues for all implementation work and generally for analysis work as well. (Other work such as consultations, meetings, e-mails, professional discussions are not tracked, obviously, so it’s important to note that time spent on issues is not expected to add up to the seven-hour workday.) Issues help us track and prioritize work that needs to be done as well as features we may or may not tackle in the future.

Code reviews are vital: apart from the number of future issues we catch that way, it helps develop important professional skills in giving and receiving criticism, helps maintain and overall familiarity with the code within the team, and helps us establish coding style and converge on other norms. Benefits aplenty.

Our workflows have evolved over time and have been passed down informally and in pieces, but have matured to the point it’s worth formalizing. We’ll describe the processes and reasoning and back this up with some diagrams for easy reference.

Issues

Over time we’ve developed a basic workflow the team generally follows that tracks issues through their lifecycle while supporting workload management and planning. The latter is very much a work in progress but adhering to the workflow builds the foundation.

We incorporate loose time tracking in our issues so we can build an understanding of our velocity and what might be a reasonable scope for a sprint.

Issue sizes

We use five issue sizes in our team. The main three are Small, Medium, and Large. Currently we do not assign a specific duration to these: they are intended to be a gut feeling and nothing more. Most issues if properly scoped should fit into one of these three.

Additionally, Trivial means a task is less than an hour from beginning to end, including creating the issue, creating a branch, implementation, testing, review, and closing. Some trivial tasks, such as a typo or a minor improvement, could be rolled into other work if somebody was already in there, in which case a simple “hey could you fix this while you’re in there” between team members is fine, but otherwise we’ll create an issue so we don’t forget about it.

The final size is Ginormous. We’ll assign this to an issue that is too big and needs to be split into separate issues.

Issue states

The default state of an issue in GitLab is “Open”, and the other is “Closed”. We define additional states through labels, which are defined at the group level in GitLab, so all of our projects use the same states and we can in fact look at the issue board for all projects, or just one. The states are:

  • Open: We’ve identified a task for future work. We may or may not work on it anytime soon, but we’re tracking it.
  • Backlog: This task is something we plan to work on in a future sprint.
  • To Do: We are planning to work on this issue in the current sprint.
  • In Progress: This is the current task of the assigned team member.
  • Review: Developer believes the work is complete and the work is to be reviewed or is in review.
  • Closed: Work has been completed or abandoned (generally the former, but either way should be clear from the issue history).

We also have On Hold but… shh… sssshhhhh. Let’s never speak of it again.

We have defined issue boards on GitLab at the team group level for all projects and in some cases at the project level. On these boards, which are like Kanban boards, we have lists defined using each of these labels. By moving the issue cards between lists we move them between states and GitLab assigns and unassigns the appropriate labels.

Issue types

Most issues will be implementation tasks, that is, code or configuration. These have no corresponding label.

We might create an issue for research work, such as investigating a Javascript framework or for design work. We’ll assign an Analysis label for these. Similarly, if we identify a problem that requires significant collaboration, we’ll stick a Discussion label on it. In both of these cases, once the analysis or discussion has concluded, we will create a separate issue for implementation (which should obviously link back to the source issue).

Issue lifecycle

  1. An issue is created as a result of developer insight, team discussions, or consultation with the client.
    • Anybody can create an issue.
    • Others may weigh in on the issue such as to request clarification, add information, or suggest design or implementation aspects.
    • The title should be short and descriptive–it will be used in the branch name in implementation.
    • The description should provide enough detail such that anybody familiar with the project will understand the issue.
    • Typically an issue will not be assigned to anybody at first. If it’s a bug and urgent, it might be assigned (such as to self) and worked on immediately.
    • Labels such as “Analysis” or “Discussion” may be assigned as appropriate.
    • Labels for issue size and state are not typically assigned at this point. In case of bugs, we will likely place in Backlog immediately, and if urgent, with approval the developer might estimate size and move to To Do or In Progress.
  2. The team identifies the issue as being appropriate for work and promotes it to the Backlog.
    • At this time the size may be estimated. Generally this is by consensus or majority vote, carried out by throwing up one, two or three fingers on the count of three with some very brief discussion to settle on the size.
    • If the size is determined to be Ginormous, we’ll need to split the issue into multiple issues. This will be a brief discussion on what those are, after which the original issue assumes the role of the first issue to be tackled and the team votes on size. After the meeting, the team lead will create the other issues and link to the original.
  3. During sprint planning the team decides whether the issue is in scope for the sprint and, if so, moves it to To Do, otherwise it stays in the Backlog.
  4. At some point during the sprint a developer will take ownership of an issue and start working on it, which they indicate by moving it to In Progress. This is easy to forget to do but important so we know what we’re all working on.
  5. If the developer finishes work, they’ll create an MR linked to this issue and move the issue to Review.
  6. If the developer puts work on hold to concentrate on another task, or is blocked for any reason, they will move the issue back to To Do.
  7. Once an MR for an issue has been approved and merged, the issue is closed.

Issue workflow

An Analysis or Discussion issue will follow the lifecycle but will skip several steps of this workflow; however, it’s important that we continue to track effort on these tasks.

  1. Assign issue to self and move to In Progress.
  2. Create a branch off of the main branch for the issue. The branch name should have the format “i(issue number)-(issue title in lower kebab case)”, so if the issue is number 32 and its title is “Implement flurble algorithm”, the branch name should be “i32-implement-flurble-algorithm”.
  3. Do some work.
  4. Log effort in issue:
    • Record time spent with at least 15-minute accuracy. More is fine if it’s easier but it is not necessary to round up or round down. Don’t bother with forensic analysis if you forget and have to come back to it later: I don’t want time tracking to waste time.
    • Record a brief outline of the work performed, if there’s anything useful to record. If the time was spent coding, “Continued coding” probably isn’t enlightening, but something like “Tried out flurble algorithm” would be.
  5. If the work is complete, create an MR and move the issue to Review.
  6. Otherwise, return to step 3 when ready.

Merge requests

Our small team has had a pretty decent informal workflow for reviewing merge requests (MRs, the GitLab equivalent of a “pull request” or “PR” on GitHub): when a team member finishes the initial work on some code, they create an MR, the next member reviews it and adds comments, the developer resolves the issues raised or continues the discussion, then the next reviewer has a look, offers comments, and so on, until all reviewers are happy and so is the developer, and then the developer rebases if necessary and the code owner merge the code. The most junior member always reviews first and the owner reviews last, and it works pretty well.

By now however we’ve had a couple of occurrences of reviewing code out of order or when it’s still in progress. As well, issues would generally remain open until somebody went in and cleaned them up, and from time to time, an MR would be idle because the handoff between reviewers didn’t quite happen. So, it’s time to formalize our workflow a bit.

The workflow

  1. The initial work is complete, so the developer pushes the latest updates to their branch and creates an MR.
  2. The developer assigns a reviewer. (With the free version of GitLab we can’t assign multiple reviewers.) They choose the next most junior developer.
  3. The assigned reviewer reviews the code and makes comments along the way, then submits the review. (GitLab’s tools are pretty good for this.)
  4. The developer addresses each comment with code changes, if appropriate, and a reply. For trivial changes such as typos, a reply isn’t necessary and the developer can resolve the comment.
  5. The assigned reviewer reviews non-trivial updates and/or discussion. If satisfied, they resolve the comment thread. If any review comments remain unresolved, return to the developer for further consideration.
  6. When all comments have been resolved, the reviewer approves the merge request and assigns to the next reviewer who then conducts a review with the same flow–return to step 3.
  7. When all reviewers have approved the code, the code owner requests a rebase if necessary, and,
  8. …if so, the developer performs one.
  9. The code owner merges.
flowchart TD

Start([Initial work complete]) --> Create[Developer creates MR]
Create --> Assign[Developer assigns reviewer]
Assign --> Review[Reviewer reviews code and makes comments along the way]
Review --> Submit{Reviewer\nsubmits\nreview}
Submit -->|Approve| Next{Assign next}
Next -->|I'm the last one| NeedsRebase{Needs rebase?}
Next --> Review
Submit -->|Request changes| Addressing[Developer addresses comments]
Addressing --> ReviewReview[Reviewer reviews updates and discussion.\nIf
satisfied, resolves thread]
ReviewReview --> Resolved{All\ncomment threads\nresolved?}
Resolved -->|Yes| Approve[Reviewer approves merge]
Approve --> Next
Resolved -->|No| ReviewFollowup[Reviewer makes any \nadditional comments
and\nsends back to developer]
ReviewFollowup --> Addressing
NeedsRebase -->|Yes| RequestRebase[Request rebase]
NeedsRebase -->|No| Merge
RequestRebase --> Rebase[Developer performs rebase]
Rebase --> Merge([Merge])

Important notes

Roles

The developer is the author of the code; the reviewers are every team member other than the developer, and the code owner is typically the team lead but may be another member who is responsible for the project.

Reviewer sequence

All team members other than the developer review a merge request. Regardless of authorship, reviews are conducted starting with the most junior members of the team, and progressing to the next most junior member until the team lead conducts the final review.

This provides several benefits:

  • Junior members gain valuable experience in analysing, evaluating, and learning from other people’s work.
  • Junior members gain valuable experience in critiquing others’ work.
  • Junior members gain valuable experience in receiving those critiques.
  • Members learn from each other’s knowledge and techniques.

All of these apply to all team members, but at the beginning of the workflow, the total value is greatest when the developer and reviewer are closer in experience: junior developers may not, for example, notice algorithmic efficiency, but can help correct style or clumsy logic or unclear comments or even spelling, saving more senior members their attention for a deeper analysis.

Draft MRs

For larger works, the developer may create an MR and flag it as Draft as work progresses. A common reason for this is to solicit reviews and advice before moving forward with further coding. In such cases the developer may request a review, but unless requested, reviews are optional while the MR is marked as a draft or work-in-progress.

Issue-MR linkage

A merge request should always link to the issue it aims to resolve, and vice-versa. When an MR is approved and merged, the corresponding issue should be closed accordingly. Ideally, this would be automated–in the past we have had to periodically clean up issues that have been resolved for some length of time, and this would often require some memory work and re-evaluation.

Linking the issues with text like “Addresses #10” in the MR description is a convention we adopted from [Bhavy](link to bio) and the issue number is interpreted by GitLab and turned into a link, so it’s pretty simple to follow that and close the issue. But it turns out that if the MR contains an issue reference embedded in a particular pattern, GitLab will automatically close the issue when the merge request is merged.

Turns out the pattern accepts “Fixes #10”, “Resolves #10”, or “Closes #10”, but not “Addresses #10” which I prefer. (The others seem presumptuous, but maybe that’s because my fixes don’t always fix.)

Time tracking

To build up and maintain an understanding of how much time we need for tasks, we track our time spent on issues and merge requests.

  1. Track implementation time on the issue and briefly record what work was performed.
  2. Track review time on the merge request.
  3. Strive for 15-minute accuracy. You can be more accurate if you want to, and there’s no need to round up or round down.
  4. Track time whenever you like, but ideally at the end of the session, and at least at the end of the day.
  5. If you forgot to track time on an issue or MR, go back and record it, and just estimate when and for how long you worked on a task–you don’t need to go looking for forensic evidence (“Lessee, I had that conversation on Slack before and then afterwards I sent that e-mail, so let’s have a look at those timestamps…”)

Discovering a new issue while working in the code

When discovering an unrelated new issue or potential issue when working on the code, it’s important to track that so we don’t lose sight of it. In some cases it might be a simple fix. The team has settled on the following:

  • If the fix is trivial we will make that change as part of this work. If it is a functional change, this must be a separate commit. If it’s a typo in a comment, no separate commit is necessary. Developers will apply their judgement here.
  • Otherwise, we’ll create a separate issue, and continue to work the original issue.

So what’s trivial?

  • Basic typos are trivial and can be fixed immediately and in-place. We’ve typically done this anyway. A separate commit is not required.
  • Out-of-date comments that can be easily cleaned up can be fixed immediately although depending on scope probably require their own commit.
  • Anything requiring changes to testing or the developer feels needs a second set of eyes on–in other words, a review–warrants its own issue.

Ultimately, err on the side of caution and create a separate issue.