watonomous.github.io

[ Electrical Division : Git Workflow ]

Created by [ Kash Pirani], last modified on Jan 21, 2020

Naming Convention

Given that we use both Jira and Gitlab, it is important that the naming of branches and commits across the two systems are consistent. Although there is supported integration of Gitlab and JIRA, we unfortunately don't have the required membership level to mesh the two services together. This is why the following naming convention will be used across all sub-teams in the electrical division.

[Branches:]

Convention: Branches should be named in the style of "userid/ticket#/description". Ex) aapirani/TEL-17/nominal_battery_voltage

Reasoning: It is important to note that Watonomous is a student design team, which means that your work has a high chance of being handed off to another member during term transitions. Furthermore, your code will be reviewed by multiple people throughout the course of its timeline. This convention achieves a few things in that regard:

This convention directly links your branch to the JIRA issue that you are trying to solve. This means that if someone is looking at the branches in the repo they are immediately able to tell what exactly that branch is trying to achieve. This helps us figure out which branches are "dead" and which are actually valid.
It tells us who the current active developer of the branch is, and gives us information about who is working on what at a quick glance.
Ideally the description should be able to concisely tell a reviewer what feature or bug the branch is trying to address. It is also helpful to the developer in the case that they have many branches, as it makes it easy to remember the purpose of each branch.
Although the convention means the branch names can get rather long, the value provided by the convention outweighs the annoyance of having to type it out. Furthermore, all modern developers should be either using an IDE that makes finding and pushing to git branches easy - or they should have autocomplete set up in their bashrc. In other words, you should only ever have to type out the full branch name once.

Convention: Feature branches are the only branches that may break this convention, and they must follow the naming style "feature/ticket#/description"

[Commits:]

Convention: Commit messages should be concise, professional, and accurately describe what was done in the commit. Commit messages will be written in the present imperative tense, and start with a verb. Ex) "Fix an integer overflow issue in the BMS code". If multiple commits achieve the same goal (ex. commits that simply change names or format code) then they should be squashed before being pushed.

Reasoning: The reasoning for this standard is fairly straightforward:

Commit messages exist to describe what has changed between the current commit and the last commit. In other words, they tell other developers what will change in the codebase should they choose to apply this commit to a branch. It is important that commits are written as concisely as possible to save both the developer's and reviewers' time.
Why squash similar commits? This standard is mostly to make merge requests and code reviews easier on other developers. Code reviews often happen in an iterative process, so it can be useful to look at a branch's diff across different commits to see what has changed since the last time you reviewed the MR. It can be frustrating to scroll through several commits that make very minor or insignificant aesthetic changes. Squashing these commits into a single commit thus summarizes all relevant changes and speeds up the review process.
Why present tense? If you have never wrote commit messages in this style, then this may take some getting used to. This is actually a suggestion from the developer's of Git itself, and their reasoning is as follows. A commit message does not exist as a journal of what an individual developer has done, but rather as a command to the codebase that changes functionality or aesthetic in someway. Relating back to the first point, a commit message should tell other developers what will happen in the case that the commit is applied (merged). Having all developers in a repository phrase their commits in this way also creates consistency across the project - which is always beneficial to both reviewers and new members to the team.

[Merge Requests]

Convention:

Merge requests shall be titled with the JIRA ticket (or tickets) that it addresses followed by a short description. Ex) "TEL-17: Determines the nominal battery voltage from reported CAN data."
The message of the merge commit shall be the same as the merge request title.

Reasoning:

The reasoning is pretty similar to why branches are named with the JIRA ticket incorporated. As a reviewer on your merge request, I want to quickly know what issues your merge request addresses and a concise summary of how it achieves that.
The merge commit message is what we actually see in master, so it is important that it follows the same conventions as the merge request title. This allows us to quickly understand what changes have been made to the repository and for what reason when looking at the repo history/git log. Changes to a repo should not be made without a documented reason (ie. JIRA ticket), so this convention also helps us enforce that.

Workflow Conventions

It is important that the electrical division as a whole adopts a common workflow. This ensures that timelines can be created accurately, that the management of sub-teams is more consistent, and that overall code quality is higher. The workflow adopted by the electrical team and associated reasoning is described below.

[Develop Branches (Gitflow) vs Just Master (Centralized)]

Convention: The Electrical division will merge pull requests directly into master rather than using a develop branch that has periodic "releases". (although if you are curious about the "release" workflow you can read more here https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow)

Reasoning:

The sub-teams in the electrical division tend to be fairly small and are not release driven. In other words, we do not work until we have gathered enough features for a release - we are constantly merging in new code to fit internal timelines and track days.
Confusion can arise in repositories when master is not actually the most up to date branch. To make it easier for developers from other teams to find and view our code, it is important that we only have a single source of truth for each project - in this case the master branch.
Develop branches are not needed to protect master from breakage. Why? All code should be tested and reviewed before it is merged into master (at least in a perfect world.) In the cases that "bad" code is merged in, only a single merge commit will need to be rolled back in order to enter a stable state again. Because we are a student design team, and are not deploying a product to external customers, this level of safety is satisfactory given the benefits we reap from a simpler workflow.

[Merge Requests]

Convention: Merge requests must be reviewed and [approved] by your manager or director before it can be merged into master. If you are a manager or director, you must have at least one approval from another developer on the team to merge your code in.

Reasoning:

Code reviews can catch bugs, stylistic anomalies, inefficiencies, and incorrect behaviour in code. This helps mitigate the amount of "bad code" that we merge into our repos.
Code reviews help the reviewer understand what changes are being made in their project, and encourage them to step outside their comfort zone and read/critique code they have not personally written. I believe this is a critical step in growing as a developer.
Although hopefully this never happens, this process of approval will help prevent malicious code or un-discussed sweeping changes from being merged in.

Convention: All commits is a merge request must be squashed upon merged, and the source branch shall be deleted from remote.

Reasoning:

It is really messy to look at the history of master and see a handful of commits that don't really describe what they do. Squashing all of the commits in a branch "cleans-up" the history of master and presents developers with one simple commit that explains how and why it changes the codebase,
A single merge commit makes it easier to revert any "bad changes." It can be painful and time consuming to work through each commit in master's history and try find the one that broke the code. Having a single commit for each feature that was merged in allows us to rollback changes to the code base on a feature-to-feature basis, rather than a commit to commit basis. This makes it easier to return to a stable version of master, as well as isolate the change at fault.
There is no point in keeping stale branches on the remote after their contents have already been merged into the repo. We are not losing any information by deleting them, as any changes in them will be present in master after merging. Ideally, we should only have branches in review or active development present on remote.
Squashing and branch deletion are already handled by Gitlab as long as you tick both the "squash commits" and "delete source branch" check boxes when creating your merge request.

[Branching]

Convention: Branches should only be pushed to remote if they are addressing a JIRA ticket. Personal develop branches and "hacks" should never reach the merge request stage.

Reasoning:

Any changes made to the repo should be well documented and discussed by the sub-team. To this end, there should not exist any remote branches that are not actively implementing a feature or fixing a bug mentioned in a JIRA Ticket.
Personal "develop" branches used to mess around with ideas or features should not be pushed to remote. The exception to this would be if you needed to share your work with another developer (but at that point you should really just have a JIRA ticket documenting what you are trying to achieve and why)
We don't want our repos filling up with undocumented "hacks" that achieve the bare minimum to get a feature working without regard for efficiency or good coding practice. Any branches like this need to have an associated JIRA ticket made, cleaned up, and merged when in a proper working state.

Convention: Feature branches may be used when multiple developers are working on a large ticket

Reasoning:

It is not reasonable to expect a multi-developer project to be incrementally merged into master (as nice as this would be though.) Sometimes it makes more sense to merge a large feature into master in one large PR.
Feature branches allow multiple developers to work on small components of a large feature and merge it into a common place without having to worry about breaking master.
A Feature branch is distinctly different from the develop branches mentioned above. While develop branches last for the lifetime of the repository and are periodically updated, feature branches only exist for the lifecycle of the feature's development. They are created with the intention to be merged into master and subsequently deleted.

Workflow Summary

A typical feature lifecycle can be described as follows (TODO - update this text with an actual block diagram):

JIRA Ticket → pull latest master & create branch → commit changes to branch until feature is completed+tested → make merge request and add relevant reviewers → change code according to feedback → resolve any merge conflicts(locally or on remote) → merge into master once approved → rinse & repeat

Comments:

+———————————————————————–+ | [] | | | | Just a question regarding the Squash functionality in GitLab (for | | MRs); are we able to write our own Squash message, or do we have to | | take the default message provided from GitLab? | | | | If we are unable to write our own squash message (when merging the | | MR) then I think it would be best that the developer squashes the | | commits themselves, so that we have a meaningful commit message | | (ticket ID, description, etc.) rather than super long and not really | | helpful commit message. Then we can simply just rebase this commit | | into the master branch. | | | | {.smallfont align=”left” style=”color: #666666; width: 98%; margi | | n-bottom: 10px;”} | | | | Posted by sbaveja at Jan 21, 2020 13:00 | | | +———————————————————————–+ | [] | | | | By default it uses the MR title as the merge commit, but it also lets | | the developer specify another message. | | | | {.smallfont align=”left” style=”color: #666666; width: 98%; margi | | n-bottom: 10px;”} | | | | Posted by aapirani at Jan 21, 2020 13:14 | | | +———————————————————————–+ | [] | | | | Okay, thats good then, much better that Bitbucket in this sense. | | | | {.smallfont align=”left” style=”color: #666666; width: 98%; margi | | n-bottom: 10px;”} | | | | Posted by sbaveja at Jan 21, 2020 13:26 | | | +———————————————————————–+

Document generated by Confluence on Nov 28, 2021 22:40

Atlassian