Skip to content

standards.rst

Latest commit

 

History

History
309 lines (213 loc) · 10.4 KB

standards.rst

File metadata and controls

309 lines (213 loc) · 10.4 KB
title:Coding Standards
description:Deis project coding standards. Contributors to Deis should feel welcome to make changes to any part of the codebase.

Coding Standards

Deis is a Python and Go project.

We chose Go because it is simple, reliable, and efficient. These are values Deis shares. Go also excels at programming systems-level tasks, with powerful and simple networking, concurrency, and testing facilities included. Our coding standards and recommendations for Go code in the Deis project are evolving, but will be added to this document soon.

We chose Python over other compelling languages because it is widespread, well-documented, and friendly to a large number of developers. Source code benefits from many eyes upon it.

The Zen of Python emphasizes simple over clever, and we agree. Readability counts. Deis also aims for complete test coverage.

Contributors to Deis should feel welcome to make changes to any part of the codebase. To create a proper GitHub pull request for inclusion into the official repository, your code must pass two tests:

make flake8

flake8 is a helpful command-line tool that combines the output of pep8, pyflakes, and mccabe.

$ make -C controller flake8
flake8

No output, as above, means flake8 found no errors. If errors are reported, fix them in your source code and try flake8 again.

The Deis project adheres to PEP8, the python code style guide, with the exception that we allow lines up to 99 characters in length. Docstrings and tests are also required for all public methods, although flake8 does not enforce this.

Default settings for flake8 are in the [flake8] section of the setup.cfg file in the project root.

make coverage

Once your code passes the style checker, run the test suite and ensure that everything passes and that code coverage has not declined.

$ make -C controller coverage
coverage run manage.py test --noinput api web
WARNING Cannot synchronize with etcd cluster
Creating test database for alias 'default'...
...............................................
----------------------------------------------------------------------
Ran 47 tests in 47.768s

OK
Destroying test database for alias 'default'...
coverage html

If a test fails, fixing it is obviously the first priority. And if you have introduced new code, it must be accompanied by unit tests.

In the example above, all tests passed and coverage created a report of what code was exercised while the tests were running. Open the file htmlcov/index.html under the project's root and ensure that the overall coverage percentage has not receded as a result of your changes. Current test coverage can be found here:

Coverage Status

Pull Request

Now create a GitHub pull request with a description of what your code fixes or improves.

Before the pull request is merged, make sure that you squash your commits into logical units of work using git rebase -i and git push -f. Include documentation changes in the same commit, so that a revert would remove all traces of the feature or fix.

Commits that fix or close an issue should include a reference like Closes #XXX or Fixes #XXX in the commit message. Doing so will automatically close the GitHub issue when the pull request is merged.

Merge Approval

Deis maintainers add "LGTM" (Looks Good To Me) in code review comments to indicate that a PR is acceptable. Any code change--other than a simple typo fix or one-line documentation change--requires at least two of Deis' maintainers to accept the change in this manner before it can be merged. If the PR is from a Deis maintainer, then he or she should be the one to merge it. This is for cleanliness in the commit stream as well as giving the maintainer the benefit of adding more fixes or commits to a PR before the merge.

Commit Style Guide

There are several reasons why we try to follow a specific style guide for commits:

  • it allows us to recognize unimportant commits like formatting
  • it provides better information when browsing the git history

Recognizing Unimportant Commits

These commits are usually just formatting changes like adding/removing spaces/empty lines, fixing indentation, or adding comments. So when you are looking for some change in the logic, you can ignore these commits - there's no logic change inside this commit.

When bisecting, you can ignore these by running:

git bisect skip $(git rev-list --grep irrelevant <good place> HEAD)

Providing more Information when Browsing the History

This adds extra context to our commit logs. Look at these messages (taken from the last few AngularJS commits):

  • Fix small typo in docs widget (tutorial instructions)
  • Fix test for scenario.Application - should remove old iframe
  • docs - various doc fixes
  • docs - stripping extra new lines
  • Replaced double line break with single when text is fetched from Google
  • Added support for properties in documentation

All of these messages try to specify where the change occurs, but they don’t share any convention. Now look at these messages:

  • fix comment stripping
  • fixing broken links
  • Bit of refactoring
  • Check whether links do exist and throw exception
  • Fix sitemap include (to work on case sensitive linux)

Are you able to guess what’s inside each commit diff?

It's true that you can find this information by checking which files had been changed, but that’s slow. When looking in the git history, we can see that all of the developers are trying to specify where the change takes place, but the message is missing a convention. Cue commit message formatting entrance stage left.

Format of the Commit Message

{type}({scope}): {subject}
<BLANK LINE>
{body}
<BLANK LINE>
{footer}

Any line of the commit message cannot be longer than 90 characters, with the subject line limited to 70 characters. This allows the message to be easier to read on github as well as in various git tools.

Subject Line

The subject line contains a succinct description of the change to the logic.

The allowed {types} are as follows:

  • feat -> feature
  • fix -> bug fix
  • docs -> documentation
  • style -> formatting
  • refactor
  • test -> adding missing tests
  • chore -> maintenance

The {scope} can be anything specifying place of the commit change e.g. the controller, the client, the logger, etc.

The {subject} needs to use imperative, present tense: “change”, not “changed” nor “changes”. The first letter should not be capitalized, and there is no dot (.) at the end.

Message Body

Just like the {subject}, the message {body} needs to be in the present tense, and includes the motivation for the change, as well as a contrast with the previous behavior.

Message Footer

All breaking changes need to be mentioned in the footer with the description of the change, the justification behind the change and any migration notes required. For example:

BREAKING CHANGE: the controller no longer listens on port 80. It now listens on
    port 8000, with the router redirecting requests on port 80 to the controller. To
    migrate to this change, SSH into your controller and run:

    $ docker kill deis-controller
    $ docker rm deis-controller

    and then restart the controller on port 8000:

    $ docker run -d -p 8000:8000 -e ETCD=<etcd_endpoint> -e HOST=<host_ip> \
    -e PORT=8000 -name deis-controller deis/controller

    now you can start the proxy component by running:

    $ docker run -d -p 80:80 -e ETCD=<etcd_endpoint> -e HOST=<host_ip> -e PORT=80 \
    -name deis-router deis/router

    The router should then start proxying requests from port 80 to the controller.
Referencing Issues

Closed bugs should be listed on a separate line in the footer prefixed with the "closes" keyword like this:

closes #123

Or in the case of multiple issues:

closes #123, #456, #789

Examples

feat(controller): add router component

This introduces a new router component to Deis, which proxies requests to Deis
components.

closes #123

BREAKING CHANGE: the controller no longer listens on port 80. It now listens on
    port 8000, with the router redirecting requests on port 80 to the controller. To
    migrate to this change, SSH into your controller and run:

    $ docker kill deis-controller
    $ docker rm deis-controller

    and then restart the controller on port 8000:

    $ docker run -d -p 8000:8000 -e ETCD=<etcd_endpoint> -e HOST=<host_ip> \
    -e PORT=8000 -name deis-controller deis/controller

    now you can start the proxy component by running:

    $ docker run -d -p 80:80 -e ETCD=<etcd_endpoint> -e HOST=<host_ip> -e PORT=80 \
    -name deis-router deis/router

    The router should then start proxying requests from port 80 to the controller.
----------------------------------------------------------------------------------
test(client): add unit tests for app domains

Nginx does not allow domain names larger than 128 characters, so we need to make
sure that we do not allow the client to add domains larger than 128 characters.
A DomainException is raised when the domain name is larger than the maximum
character size.

closes #392