openATTIC Contributing Guidelines

Please see Contributing Code to openATTIC for details on the process of how to contribute code changes.

The following recommendations should be considered when working on the openATTIC code base.

While adhering to these guidelines may sound more work in the first place, following them has multiple benefits:

  • It supports the collaboration with other developers and others involved in the product development life cycle (e.g. documentation, QA, release engineering).
  • It makes the product development life cycle more reliable and reproducible.
  • It makes it more transparent to the user what changes went into a build or release.

Some general recommendations for making changes and for documenting and tracking them:

  • New Python code should adhere to the Style Guide for Python Code (PEP 8) with the exception of using 100 instead of 80 characters per line. Use the flake8 tool to verify that your changes don’t violate this style before committing your modifications. Also, please have a look at the Django coding style guides as a reference.
  • Existing code can be refactored to adhere to PEP 8, if you feel like it. However, such style changes must be committed separately from other code modifications, to ease the reviewing of such pull requests.
  • For JavaScript code, we use ESLint using the eslint-loader to perform automated syntax and style checks of the JavaScript code. ESLint is configured to adhere to the official AngularJS style guide. Please consult this style guide for more details on the coding style and conventions. The configuration files for these WebUI rules can be found in webui/.eslintrc.json and webui/webpack.eslintrc.json.
  • Every bug fix or notable change made to a release branch must be accompanied by a JIRA issue. The issue ID must be mentioned in the summary of the commit message and pull request in the following format: <summary> (OP-xxxx).
  • Pull requests must be accompanied by a suggested CHANGELOG entry that documents the change.
  • New features and other larger changes also require a related JIRA issue that provides detailed background information about the change.
  • Code and the related changes to the documentation should be committed in the same change set, if possible. This way, both the code and documentation are changed at the same time.
  • Write meaningful commit messages and pull request descriptions. Commit messages should include a detailed description of the change, including a reference to the related JIRA issue, if appropriate. “Fixed OP-xxx” is not a valid or useful commit message! For details on why this matters, see The Science (or Art?) of Commit Messages and How to Write a Git Commit Message
  • When resolving a JIRA issue as fixed, include the resulting git revision ID or add a link to the ChangeSet or related pull request on BitBucket for reference. This makes it easier to review the code changes that resulted from a bug report or feature request.

Documenting Your Changes

Depending on what you have changed, your modifications should be clearly described and documented. Basically, you have two different audiences that have different expectations on how and where you document your changes:

  • Developers that need to review and comment on your changes from an architectural and code quality point of view. They are primarily interested in the descriptions you put into the git commit messages and the description of your pull request, but will also review and comment on any other documentation you provide.
  • End users or administrators that use openATTIC and need to be aware of potential changes in behaviour, new features or important bug and security fixes. They primarily consult the official documentation, release notes and the CHANGELOG.

Changes that should be user-visibly documented in the CHANGELOG, release notes or documentation include:

  • Bug/security fixes on a release branch.
  • User-visible changes or changes in behavior on a release branch. Make sure to review and update the documentation, if required.
  • Major changes / new features. In addition to the CHANGELOG, these must be described in the documentation as well. See Working on the openATTIC documentation for details on how to update the openATTIC documentation.

Minor or “behind the scene” changes that have no user-visible impact or do not cause changes in behavior/functionality (e.g. improvements to build scripts, typo fixes, internal code refactoring) usually don’t have to be documented in the CHANGELOG or the release notes.

Trust your judgment or ask other developers if you’re unsure if something should be user-visibly documented or not.

Don’t worry too much about the wording or formatting, the CHANGELOG and Release Notes will be reviewed and improved before a final release build anyway. It’s much more important that we keep track of all notable changes without someone having to trawl JIRA or the commit messages prior to a release.

Signing Your Patch Contribution

To improve tracking of who did what, we use the “sign-off” procedure introduced by the Linux kernel. The sign-off is a simple line at the end of the explanation for the patch, which certifies that you wrote it or otherwise have the right to pass it on as an open-source patch.

The rules are pretty simple: if you can certify the following:

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
660 York Street, Suite 102,
San Francisco, CA 94110 USA

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

then you just add the following line below your commit message and pull request saying:

Signed-off-by: Random J Developer <[email protected]>

using your real name and email address (sorry, no pseudonyms or anonymous contributions).

Using git, this can be performed by adding the option --signoff to your commit command.

If you like, you can put extra tags at the end:

  1. Reported-by: is used to credit someone who found the bug that the patch attempts to fix.
  2. Acked-by: says that the person who is more familiar with the area the patch attempts to modify liked the patch.
  3. Reviewed-by:, unlike the other tags, can only be offered by the reviewer and means that she is completely satisfied that the patch is ready for application. It is usually offered only after a detailed review.
  4. Tested-by: is used to indicate that the person applied the patch and found it to have the desired effect.

You can also create your own tag or use one that’s in common usage such as Thanks-to:, Based-on-patch-by:, or Mentored-by:.

Merging Pull Requests

The following steps should be performed when you’re reviewing and processing a pull request on BitBucket:

  1. A developer fixes a bug or implements a new feature in a dedicated feature branch. If required, he documents the changes in the documentation (for end-users) and the git commit messages (including the related Jira issue ID and a Signed-off by: line as outlined in chapter Signing Your Patch Contribution)
  2. The developer creates a new Pull Request on BitBucket as described in chapter Submitting Pull Requests. The Pull Request description should include a detailed description of the change in a form suitable for performing a code review, summarizing the necessary changes. The description should also include a text suitable for inclusion into the CHANGELOG, describing the change from an end-user perspective.
  3. After the pull request has been reviewed and approved, you perform the merge into the master branch using the BitBucket merge functionality.
  4. Use a “merge” commit, not a “squash” commit for merging pull requests via BitBucket.

Backport commits

The following steps should be performed when you want to backport a fix to a stable release branch:

  1. Ensure that the commits you want to backport exists on master (original pull request is merged to master)
  2. Update your upstream repo: git fetch upstream
  3. Create a branch from the stable release branch: git checkout -b OP-<issue_id>-backport upstream/2.x
  4. Cherry pick the commits, using -x option: git cherry-pick -x <sha-1>
  5. Adapt the CHANGELOG
  6. Run all tests
  7. Submit a pull request to the 2.x stable release branch (title should be prefixed with “[2.x]”)

Error Handling in Python

A few notes about error handling in openATTIC.

Good error handling is a key requirement in creating a good user experience and providing a good API. In our opinion, providing good errors to users is a blocker for releasing any non-beta releases of openATTIC.

Assume all user input is bad. As we are using Django, we can make use of Django’s user input validation. For example, Django will validate model objects when deserializing from JSON and before saving them into the database. One way to achieve this is to add constraints to Django’s model field definitions, like unique=True to catch duplicate inserts.

In general, input validation is the best place to catch errors and generate the best error messages. If feasible, generate errors as soon as possible.

Django REST framework has a default way of serializing errors. We should use this standard when creating own exceptions. For example, we should attach an error to a specific model field, if possible.

Our WebUI should show errors generated by the API to the user. Especially field-related errors in wizards and dialogs or show non-intrusive notifications.

Handling exceptions in Python should be an exception. In general, we should have few exception handlers in our project. Per default, propagate errors to the API, as it will take care of all exceptions anyway. In general, log the exception by adding logger.exception() with a description to the handler.

In Python it is easier to ask for forgiveness than permission (EAFP). This common Python coding style assumes the existence of valid keys or attributes and catches exceptions if the assumption proves false. This clean and fast style is characterized by the presence of many try and except statements. The technique contrasts with the LBYL style common to many other languages such as C.

When calling system utilities or call external libraries, raise exceptions if appropriate to inform the user of problems. For example, if mounting a volume fails, raise an exception. From the Zen Of Python:

Errors should never pass silently. Unless explicitly silenced.

Distinguish user errors from internal errors and programming errors. Using different exception types will ease the task for the API layer and for the user interface:

  • Use NotImplementedError in abstract methods when they require derived classes to override the method. Have a look at the official documentation.
  • Use ValidationError in an input validation step. For example. Django is using ValidationErrors when deserializing input.
  • In case a NotImplementedError is not appropriate, because it is intentional not to implement a method and a ValidationError is not appropriate, because you’re not validating any input, you can use a NotSupportedError. For example, if a file system does not support shrinking, you can use this exception here. They will be converted to a 400 status code in the API.
  • Standard Python errors, like SystemError, ValueError or KeyError will end up as internal server errors in the API.
  • Assert statements can help, if you want to protect functions from having bad side effects or return bad results.

In general, do not return error responses in the REST API. They will be returned by the openATTIC error handler exception.custom_handler. Instead, raise the appropriate exception.

In a Python function, in case of an error, try to raise an exception instead of returning None. Returning None in this case forces your caller to always check for None values.

Database migrations

Please follow the standard guidelines for adding database migration for Django 1.8+. Even, if you changed a NoDB model that don’t write any data into the database. This ensures a consistent table schema for future Django upgrades.