... | ... | @@ -34,17 +34,17 @@ cd stork |
|
|
|
|
|
- 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](https://gitlab.isc.org/isc-projects/stork/-/boards?scope=all&utf8=%E2%9C%93&state=opened&milestone_title=Stork-0.1) 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.
|
|
|
- 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](https://gitlab.isc.org/isc-projects/stork/-/boards?scope=all&utf8=%E2%9C%93&state=opened&milestone_title=Stork-0.1) 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 a 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 reviewer: go to Issues->Board page. Look for an issue in the 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 developer: look for issues that are assigned to you. Do your best with addressing the comments. Put a witty response to snarky 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.
|
|
|
- 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. Recenty gitlab versions also have Approve button. Use it.
|
|
|
|
|
|
- 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.
|
|
|
- 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 about your own arguments. Are you trying to push your point of view, simply because you like it? It's an art of compromise.
|
|
|
|
|
|
# Merging code
|
|
|
|
... | ... | @@ -52,7 +52,7 @@ cd stork |
|
|
|
|
|
- Before merging, please run unit-tests and linters on your own (useful commands: `rake unittest_backend` and `rake lint_go fix=true`, also `rake lint_ui`)
|
|
|
|
|
|
- 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:
|
|
|
- Using rebase button on the MR page is a very nice way of rebasing your branch. If there are conflicts, here's how you can rebase the code manually:
|
|
|
```console
|
|
|
# commit everything on your branch
|
|
|
git checkout master
|
... | ... | @@ -64,17 +64,17 @@ 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.
|
|
|
- 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.
|
|
|
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.
|
|
|
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).
|
|
|
|
... | ... | @@ -82,7 +82,7 @@ If you really like to, you can squash your own commits. Nobody in the Kea team d |
|
|
|
|
|
# Changelog
|
|
|
|
|
|
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.
|
|
|
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 the 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.
|
|
|
|
... | ... | @@ -105,7 +105,7 @@ Branch names must start with an issue number. Yes, there has to be an issue, eve |
|
|
|
|
|
# Using labels
|
|
|
|
|
|
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)?
|
|
|
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 a 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.
|
|
|
|
... | ... | |