Skip to content

Ways of Working

Git branching model

We have adopted the GitFlow Branching Model in order to support both Gaffer v1 and v2: GitFlow Branching Model

Issues

Where possible a pull request should correlate to a single GitHub issue. An issue should relate to a single functional or non-functional change - changes to alter/improve other pieces of functionality should be addressed in a separate issue in order to keep reviews atomic. The reasoning behind code changes should be documented in the GitHub issue. All resolved issues should be included in the next GitHub milestone, this enables releases to be linked to the included issues. If a code change requires users of Gaffer to make changes in order for them to adopt it then the issue should be labelled 'migration-required' and a comment should be added similar to:

### Migration Steps

[Description of what needs to be done to adopt the code change with examples]

Workflow

  • Assign yourself to the issue
  • Create a new branch off develop using pattern: gh-[issue number]-[issue-title]
  • Commit your changes using descriptive commit titles
  • Check and push your changes
  • Create a pull request (PR) to merge your branch into develop, prefixing the PR title with "Gh-[issue number]: "
  • If you named the branch and PR correctly, the PR should have "Resolve #[issue-number]" automatically added to the description after it is made. If it doesn't, then please add the issue it will resolve as a "Linked issue"
  • If there is a significant change, please follow the same process to document the change in gaffer-doc
  • The pull request will be reviewed and following any changes and approval your branch will be squashed and merged into develop
  • Delete the branch
  • The issue will be closed automatically

Pull Requests

Pull requests will undergo a review by a Gaffer committer to check the code changes are compliant with our coding style. This is a community so please be respectful of other members - offer encouragement, support and suggestions.

As described in our git branching model - please raise pull requests to merge your changes in our develop branch.

When pull requests are accepted, the reviewer should squash and merge them. This is because it keeps the develop branch clean and populated with only merge commits, rather than intermediate ones. As well as this, it makes everyone's job reviewing pull requests easier as any insecure and unreviewed intermediate commits are not included into the develop branch.

Please agree to the GCHQ OSS Contributor License Agreement before submitting a pull request. Signing the CLA is enforced by the cla-assistant.

Documentation

As mentioned before, any significant changes in a PR should be accompanied with an addition to Gaffer's documentation: gaffer-doc. Smaller changes should be self documented in the tests. With this approach, any large feature or change has user friendly documentation, whereas technical or implementation details are documented for developers by the tests.

Coding style

Java

Please ensure your coding style is consistent with the rest of the Gaffer project and the Google Java Style Guide. Your changes should pass the checkstyle and spotless plugins that are part of the continuous integration pipeline and check for code formatting and licenses. Before you push your changes you can check the checkstyle plugin passes with mvn checkstyle:check and check the spotless plugin passes with mvn spotless:check.

Python

Please ensure your coding style is consistent with the rest of the Gaffer project and the PEP 8 Style Guide. However, there are a few exceptions to the standards set by PEP8: * Module level imports at the top of the file - this will not be enforced but is recommended where it does not cause issues with the code generated by Fishbowl. * Max line length of 79 characters - the max line length that will be enforced in this project has been increased to 100 characters.

Before you create a PR for your changes you can use autopep8 to check and fix any styling issues. The following can be run which will take into account the rule exceptions mentioned above. autopep8 --exit-code -r -i -a -a --max-line-length 100 --ignore E402 .

Javadoc

Ensure your java code has sufficient javadocs explaining what the section of code does and the intended use of it. Javadocs should be used in addition to clean readable code.

In particular: * All public classes (not required for test classes unless an explanation of the testing is required) * public methods (not required if the functionality is obvious from the method name) * public constants (not required if the constant is obvious from the name)

Tests

  • All new code should be unit tested. Where this is not possible the code should be invoked and the functionality should be tested in an integration test. In a small number of cases this will not be possible - instead steps to verify the code should be thoroughly documented.
  • Tests should cover edge cases and exception cases as well as normal expected behavior.
  • Keep each test decoupled and don't rely on tests running in a given order - don't save state between tests.
  • For a given code change, aim to improve the code coverage.
  • Unit test classes should test a single class and be named [testClass]Test.
  • Integration test classes should be named [functionalityUnderTest]IT.
  • Tests should be readable and self documenting.
  • Each test should focus on testing one small piece of functionality invoked from a single method call.
  • Tests should use JUnit 5 and assertJ.
  • We suggest the following pattern:
@Test
public void should[DoSomething|ReturnSomething] {
    // Given
    [Setup your test here]

    // When
    [Invoke the test method]

    // Then
    [assertThat the method did what was expected]
}

Gaffer 2

During the Gaffer 2 development process there was a v2-alpha branch, which acted as the develop branch for changes staged for Gaffer 2. This branch is no longer in use and can be ignored.

Last update: September 13, 2023
Created: May 6, 2022