This page describes the Stork project workflow. Everyone who contribute to project is expected to read it at least once and follow it. You are not special, the process applies to everyone. If you don't like it, please propose changes and discuss it with the team.
- create an account if you don't have it yet.
- go to gitlab.isc.org, click on your user icon in right top corner and click settings from the drop menu. Then click ssh keys icon on the left panel.
- There are links there that explain how to generate your keys if you somehow haven't done that already. Anyway, the idea is to paste the content of your ~/.ssh/key-name.pub file there.
- As a completely optional step, I configured my ssh client to use separate key for gitlab, by adding this to my ~/.ssh/config file:
Host gitlab.isc.org IdentityFile ~/.ssh/id_ed25519_gitlab
Set up your git repository
To set up a new repository do:
git clone email@example.com:isc-projects/stork.git cd stork
Adding an issue
- If you have something new to work on, create an issue in gitlab: go to https://gitlab.isc.org/isc-projects/stork. Make sure it's not assigned to any milestone, unless you discussed this with the project manager and he is ok with adding it directly to the current milestone. We have a process to triage unassigned tickets weekly and assign them to specific milestone.
Working on an issue
Once you start working on an issue, assign the issue to yourself. You should also assign Doing label. There are two ways of doing that. First, you can add the label manually when browsing an issue. Alternatively, you can go to https://gitlab.isc.org/isc-projects/stork/boards and drag and drop your issue to appropriate stage.
Open the issue page, e.g. https://gitlab.isc.org/isc-projects/stork/issues/3 and click create a MR. A good trick is to click on the triangle button to get the extended create MR menu. In particular, you can use a shorter branch name. The branch name MUST start with the issue number.
git pull. Note the branch that was just created will be there. Check it out and start working on the code. If you dislike the branch name being too long, see the trick about about using shorter name. Also, if you have bash-completion package installed (and using bash), you can type: git checkout 3, and hit tab. The full name of the branch will be completed for you.
Commit your changes, push it on that branch.
Once you are done, do the following: unassign yourself from the issue and MR, remove "Doing" label, add "Review" label. The doing and review labels should be put on both the issue and the MR. This indicates the MR is ready for a review. This looks like a duplication of work, but it serves different purposes. The issues are tracked using a board (see this Stork 0.1 board for example). Once you assign a label to an issue it is shown in the appropriate column. Now, the review label on MR indicates that particular one is ready for review. The illusion of duplication disappears when there are multiple MRs assigned to a single issue.
Reviewing an issue
as a reviewer: go to Issues->Board page. Look for an issue in review state that has no assignee. Assign to yourself. Review. Put some snarky comments. Add some more comments. Once you're done, reassign back to the developer.
as a developer: look for issues that are assigned to you. Do your best with addressing the comments. Push your improvements to the branch. Once done reassign back to the reviewer. DO NOT MERGE until the reviewer says the code is ready.
as a reviewer: Once all your comments are addressed, put a note that the code is ready to merge, open the MR, edit it and click on "remove WIP status". Removal of the WIP status is a clear indication the code is ready to go.
If you can't reach an agreement after several review rounds, you can do one of three things: ask for a third opinion (any developer will do), ask the manager to solve the problem or perhaps rethink if this is the right approach. As disappointing as it may be, sometimes abandoning the changes and starting from scratch may be the best option in the long term. Also, think on your own arguments. Are you trying to push your point of view, simply because you like it? It's an art of compromise.
Stork project is currently set up in a way that allows only fast-forward merges. This is not the only possible option, but it works well for other projects (Kea, ISC DHCP) and it looks like the right way to go. It keeps the repo history much cleaner. If you complain about tons of merge conflicts, you tried to push a ticket that was too big.
Before merging, please run unit-tests and linters on your own (useful commands:
rake lint_go fix=true, also
Using rebase button on MR page is a very nice way of rebasing your branch. If there are conflicts, here's how you can rebase the code manually:
# commit everything on your branch git checkout master git pull git checkout <your-branch> git rebase master # solve conflicts git rebase --continue git rebase --skip # if there are specific commits that you decided to skip, e.g. because they're already on master
Once you merge your changes to master, gitlab should close the merge request and the issue.
Make sure the Changelog is updated.
GIT commit logs
In Kea we used to start every commit log with
[#Issue number,!MR number], but as a result of the Stork discussions, we're looking at going with a simplified approach. The proposal is to use
[#Issue number]. This is something that can be fully automated with a pre-commit hook in gitlab, so you won't need to type it manually.
Commit logs should be descriptive. They are addressed at developers, so code familiarity by the reader is assumed, but please be considerate. "Minor changes" is not a good description. "Fixed typo in addHost()" is better.
Since we sometimes get code from places outside of our control (MRs on gitlab, PRs on github), we cannot assume the [#Issue number] information will always be available.
We don't do commit squashes in general. Sometimes the reviewer pushes fixes or we get commits from contributors (it's essential to get those preserved. Users being able to point to their code and brag about it is one of the reasons they submit patches).
If you really like to, you can squash your own commits. Nobody in the Kea team did that.
Changelog entries are required for every change that is visible by a user. This covers things like new features (they may want to start using it), bug fixes (they may be affected and a fix may be the reason to upgrade), changes to build process (they may need to install new software dependency or, even when the dependency installation is automated, they should be aware that new piece of software will appear in their system) or documentation. Changelog should be modified on a branch and then merged to master. This is the same change as everything else, and should go through the usual review process.
The Changelog entries will be used as part of the release notes. Please make sure the text is descriptive and understandable by users, who may not be experts or don't follow developers' discussions.
We previously put SHA, gitlab issue and MR numbers in, but right now we settled for putting Gitlab issue only. It's enough to get all the related data.
An example changelog entry looks like this:
1662. [bug] marcin Prevent deadlock in the Kea DHCP servers caused by allocating memory in the system signal handler. The issue was found on CentOS 7.6, but could possibly affect Kea running on any other OS. (Gitlab #796)
Branch names must start with an issue number. Yes, there has to be an issue, even if you're doing an experiment and are uncertain if the code will ever be merged. It takes 30 seconds to create one. Tomek wants to be able to dig up what you were working on when writing release notes, even if ultimately the code didn't make it to master.
Please try to assign some labels to your issue. If there isn't any label that's appropriate, please consider creating one. Once the project matures, it's sometimes very useful to find all issues related to certain aspect (e.g. what has changed in build procedures between 0.1 and 0.2)?
Violet labels are for priority. There are four priority labels: critical, high, medium and low.
Green labels designate type and state of the issue (Doing, Review, QA needed, etc)
Red labels are for things that should stand out. Currently there's one label: bug.
This is just a proposal. Tomek's idea was to use dark colors for important things and light colors for less important aspects. If you don't like it, we can come up with an alternative naming/coloring scheme. This is working well in Kea project.
PROPOSAL ONLY (This section is being discussed)
The usual issue life cycle is as follows: opened by a reporter, a dev looks at the problem and comes up with some code or doc changes, which are later reviewed. If reporter is part of ISC team, he/she is often also a reviewer, but these two roles in principles are different. Once reviewer and dev agree that the code is ready, the change is merged and ticket is closed. However, this process has one flaw: it doesn't take into consideration whether the proposed fix actually addressed the problem raised by the original reporter.
The alternative - to ask for confirmation - has many flaws. The reporter may not be skilled enough to know how to pull a branch from git, may be unwilling or unable to compile it or more likely be simply unresponsive. This would leave us with many tickets stuck in almost-done state. So we chose to use the approach of merging and closing, with reopen as an escape clause. Sometimes we also put a note "if the solution doesn't work, please reopen". This covers 99% of the cases. Let's talk about the remaining 1%.
Someone reports a problem, we look at it, come up with a solution that we think addressed the problem, merge it and close. The reporter looks at the solution and discovers it doesn't solve the problem and reopens. There may be several reasons for this:
- the fix didn't work (or works in some cases, such as our unit-tests, but doesn't in the real life deployment as experienced by reporter)
- the dev didn't understand the core of the problem (this may be caused be inadequate or incomplete description of the problem)
- the reporter doesn't like the solution and would like to get something different
There may be other reasons. In all 3 cases, the major point is that the original problem remains unsolved and there is more to be done here. What should be done in such cases? Here are couple proposals:
APPROACH A - In case of reopen, QA gets involved and determines whether the proposed solution is ok and sufficient or not. Once dev has his updated solution (after reopen) ready to go, QA has to verify it before it's merged. This is the safest approach, but it may become very tedious. QA is busy with their normal work, so this would in general slow down dealing with reopens.
APPROACH B - You can only close a ticket once. If there's a reopen, a third person needs to get involved and independently confirm that the solution is valid and complete. This is kind of natural extension of the process we have for review where if dev and reviewer cannot reach an agreement, they ask for a third opinion.
APPROACH C - If you don't like A and B, write your proposal here.
There's also a related question of handling complex fixes. A ticket calls for something that's complex to do, e.g. ensure that new parameter is consistent with existing configuration. A dev comes up with a solution that improves the situation, e.g. does some extra checks that reject some, but not all invalid values. The full solution requires additional tickets. The question is whether to close the original one or not.
- PRO: fewer open tickets
- CON: external references
- CON: the reporter may be frustrated
KEEP IT OPEN
- PRO: reporter more satisfied (his problem is being worked on)
- CON: more tickets open
- CON: potential for having multiple changelog entries with the same ticket (can be mitigated with new tickets perceived as extra sub-tasks)