Iroha working agreement

1 Tools and processes

In Hyperledger Iroha we have following means to manage development issues:

1.1 Primary tool

JIRA tracking software, deployed at http://soramitsu.atlassian.net It contains following types of issues:

• Epics
• User Stories
• Tasks 
• Bugs

1.2 Secondary tool 

GitHub issues, at https://github.com/hyperledger/iroha/issues

1.3 Keeping consistency

Each week, Community Manager conducts grooming of backlog in primary tool and checks the consistency of issues. This also involves copying issues from GitHub to JIRA and updating their status, so that community may know the state of issue.

2 Version control system

2.1 GitHub

To store the changes and sync codebase between developers, we use git version control system.

2.2 Contribution process

Contibution to Iroha project is done via pull request to develop branch. Each pull request should be reviewed and approved by at least two developers from maintainers team.

2.3 Notion of a trunk

Trunk branch is a branch that is shared between developers for mutual work and collaboration. An example of trunk branch is develop. A process to commit to trunk is rebase-merge strategy. It means that:

1. feature should be rebased onto trunk branch and
2. after it is merged to trunk

2.4 Branch types

2.4.1 Master

Master branch contains all release versions of project. Important note: master branch is not allowed to have changed history (rebase, squash, amend, etc.) at any time. Contribution to master branch is done by merge.

2.4.2 Develop

Branch contains current general development state of project. Develop contains working and tested code. API might be unstable and changed a lot in a short period.

2.4.3 Feature

Most used type of branch for contributions. Feature represents a new functionality, being added into the system, which is generally a user story or task.

Feature branch is created from Develop branch. This type of branch is a trunk. To finish development of feature in CVS:

1. Rebase onto develop
2. Force push with rebased develop to feature branch
3. Check all GitHub steps passed
4. Merge into develop in interactive mode

2.4.4 Fix

Fix branch resolves defects which were revealed after accepting feature. Fix branch should contain:

• Test that covers problem. Test is required to prevent use case(-s) which may trigger defects in feature.
• Code that resolves defect.

2.4.5 Hotfix

Hotfix is created when a critical or severe defect was discovered in release, which must be fixed immediately. Hotfix branch should contain:

• Code that resolves defect

Hotfix should be merged both in master and develop branch. After merging it is required to create issue/task to check if defect remains with test. Hotfix should be approved by community manager or lead developer from Iroha maintainers team.

2.4.6 Refactoring

Branches that resolve technical debt in the project should be prefixed with “refactoring/”.

2.5 Merging with develop

After the code is reviewed by a maintainer, and any requested changes were made, the team of maintainers is rebasing and merging it, rather than only merging it to keep git tree tidy and beautiful. Process of rebase and merge is shown at http://soramitsu.atlassian.net/wiki/spaces/IS/pages/11173889/Rebase+and+merge+guide

2.6 Commit rules

• 2.6.1 The first line should always be 50 characters or less and then it should be followed by a blank line.
• 2.6.2 Each commit should represent one type of change. For example if you’re working on re-formatting Iroha code, don’t commit the files you work on one at a time. Edit them all, then add them all to one commit, with the commit message “Format code.”
• 2.6.3 Make your commit message as descriptive as possible. Include as much information as you can. Explain anything that the file diffs themselves won’t make apparent.
• 2.6.4 Use present simple rather that past simple to describe changes. And any present tense in general. Rather than writing messages that say what a committer has done — it's better to consider these messages as the instructions for what is going to be done after the commit is applied on the repo. So write a message like this: "Fix bug#1234" instead of "Fixed bug #1234".

3 Continuous Integration

3.1 Circle CI and Codecov.io

Circle CI is used for checking compiling time errors and to run the tests to measure coverage in Codecov.io. Each commit to the repository triggers build in Circle CI. Build status is shown per each commit in GitHub commit history, as well status per regression tests.

4 Pull requests

4.1 Opening pull requests

Developers from maintainers team cannot open pull requests 
until all tests are passed.

4.2 Pull requests notes

Pull requests notes are required because they start the discussion of technical solution and help reviewer and developer to understand code and requirements better. Usually, a developer creates a checklist of items, so that it explains what design decisions were made in details.

4.3 Number of approvals before PR is accepted

Two

4.4 How to select reviewers

Based on GitHub statistics for code changes. Ask people, who might be good at it, and worked with that code segment. Do not ask people to review
 if they are assigned to 3 and more PRs currently.

4.5 How much review rounds are allowed?

Infinite

4.6 When the review will end?

Each review comment should be marked as either:

1. defect
2. technical debt
3. suggestion to improve

#1 and #2 cannot be ignored. #1 should be fixed, and #2 should be either fixed or put into backlog after confirming with project manager. #3 should be considered as a suggestion only, if this articulated as suggestion.

4.7 Review comments

Should be without a negative emotions, or a long discussion. In should refer either to code line, code segment or to the design decision in general. Each comment should say explicitly if it reveals a defect, a technical debt occurrence or just a suggestion to improve.

4.8 Who assigns PR labels?

Label	Person
needs review	PR opener in case when a first review or correction was made
needs correction	Reviewer only
accepted	Second reviewer only
candidate for closing	Community Manager

4.9 What is their purpose?

Is solves the problem to track state of reviews and also the history of changes.

4.10 Status flows and their assignment

4.10.1 Good flow

{needs review, accepted} || {needs review, needs correction, needs review, accepted}

Is generally expected from developers from maintainers team,
 in case when the requirements are clear, design was discussed and seems evident and convincing.

4.10.2 Bad flow

{needs review, needs correction, needs review, …, accepted}

Is not expected from developers from maintainers team,
 but is normal to happen with contributors from open-source community. 
In this case it is important to help them with inadvertent technical debt. However — we should reflect on each reason of pull requests, that take more than 2 cycles of review, in order to prevent this from happening inside maintainers team.

4.10.3 Troublesome flow

{needs review, needs correction, accepted} || {needs review, accepted}

Miss of review stage of the corrections should not occur. Instant approval indicated either about bad review process when the size of code does not match, or about personal preference, which should be also discarded — we should review everyone’s work as objective as we can.

4.10.4 Outdated flow

{needs review, needs correction, …, candidate for closing}

In case when:

• pull request is opened for single month without any activity, like review or discussions, or
• review corrections are not fixed for 14 days The pull request is a candidate to be closed, if after change of status and request for the opener to clarify the status no activity happens for 24 hours.

4.11 What to review

We try to review all code changes. But you can get most of the benefits of code reviews by following the 80:20 rule: focus reviews on high risk code, and high risk changes.

4.11.1 High risk code:

• Network-facing APIs
• Plumbing (framework code, security libraries….)
• Critical business logic and workflows
• Command and control and root admin functions
• Safety-critical or performance-critical (especially real-time) sections
• Code that handles private or sensitive data
• Old code, code that is complex, code that has been worked on by a lot of different people, code that has had a lot of bugs in the past – error prone code

4.11.2 High risk changes:

• Code written by a developer who has just joined the team
• Big changes
• Large-scale refactoring (redesign disguised as refactoring)

4.11 How to review

• Review fewer than 200–400 lines of code at a time
• Aim for an inspection rate of fewer than 300–500 LOC per hour
• Be sure that authors annotate source code before the review begins
• Foster a good code review culture in which finding defects is viewed positively

5 Seven technical debt occurrences the team must always fix

1. TODO, FIXME, etc. left without a status (name, date, ticket number)
2. Unhandled throw.
3. Thrown exception.
4. Always use full path in include.
5. Angle brackets for system libraries and quotation marks for own libs in include statements.
6. Correspondence of docs with code state.
7. Unformatted code.