DevOps - Git Conventions

Owned by Megan O'Flynn
Last updated: Nov 01, 2023 by Coldstream, George (STFC,RAL,SC)
3 min read

Note, these are guidelines used by the SCD Cloud Operations Group. They are being presented externally as they may be useful for those developing their own software, they are not being provided with the intention of the Cloud Team reviewing your code - unless you are contributing to one of our repositories.

Contributing

General advice when it comes to contributing to a repository, please first discuss the change you wish to make via issue, email, or any other method with the owners of this repository before making a change.

 

The DevOps and Ops team maintain several GitHub repositories: UKRI Science and Technology Facilities Council

We encourage anyone to contribute to our STFC Cloud repositories. But to make sure your contributions can be merged quickly, please read through and follow our coding conventions carefully before raising a pull request.

Making Pull Requests

Guidance

Reviewing a PR is hard - its important make your PRs easy to review by doing the following:

1. Make Small PRs

Ensure that the PR you’re making is small and only changing one thing - if possible. If you have a task/feature that is quite big - break the task down into smaller chunks and make PRs for each chunk.

2. Write good PR descriptions

Make it clear what the PR is doing and what approach you took and why. Using diagrams can help!

3. Remove/Anonymise Sensitive Info and any test files

Treat anything in a PR as publicly available and act accordingly. Use .gitignore to prevent git from tracking these files https://www.atlassian.com/git/tutorials/saving-changes/gitignore

Pull Request Process

Making PRs (Pull Request) are the way to make changes to repos. It’s important to make small PRs so that

  1. Ensure any install or build dependencies are removed before the end of the layer when doing a build.

  2. Ensure that any sensitive information is removed or anonymized - i.e. usernames/passwords or anything that the Cloud Ops Team wouldn't want exposed publicly - talk to repo maintainers if unsure

  3. Ensure that the PR includes updates to the README.md or any other documentation in the repo or on confluence with details of the changes you're making if it changes the behaviour of the tool/program

  4. Ensure all commits and your PR name includes a acronym prefix to indicate what the commit/PR is for - one of:

    • BUG: Bug fix

    • DOC: Documentation changes/fixes

    • ENH: Enhancement

    • MAINT: maintenance commit (refactoring, typos, etc)

    • REV: revert an earlier commit

    • STY: style fix (whitespace, PEP8)

    • TST: addition or modification of tests

  5. You may merge the Pull Request in once you have the sign-off of two other developers, or if you
    do not have permission to do that, you may request the second reviewer to merge it for you.

Writing Commits Best Practice

Best practice for writing commits:

  • Make sure that the commits only make one 'atomic' change - make it as small as possible

  • Always include a description for the commit

  • Limit commit name to 50 characters

Reviewing Pull Requests

As the reviewer you are responsible to maintain code quality and reliability.

You need to:

  • understand what the PR is trying to achieve

  • understand what code is being changed and look at potential side-effects

  • evaluate the approach taken - ensure it’s extensible

  • find any bugs that may be introduced

We can make this easier by using linters, tests and code-coverage checks

If a repo doesn’t have any GitHub actions for linter checks, test checks or code-coverage checks - please add this functionality - especially if the repo is important!

Guidance

1. Ask Questions!

if you’re unsure about any of the above points - ask questions - you can do this through Slack/Email or through the PR review itself.

2. Read the Doc-strings/Comments

The first thing you should do is read the comments and docstrings added. If there aren’t any and you think there should be - raise an issue!

Comments and docstrings aren’t checked by automated checks - read them and make sure they make sense - raise issues if they don’t. Reading them will give you a better idea of what the PR is trying to achieve

3. Check for sensitive data/Testing files

If you see a file used for testing or any redundant files - raise an issue and make sure it’s deleted - this reduces the bloat for the repo

If you see that any sensitive data in the PR - tell Cloud Ops Team immediately

4. Beware of Scope Creep!

Requesting too many changes may lead to PR never getting merged - especially if these changes are off-topic/unrelated to the initial PR. Make separate issues for these changes if they don’t directly relate to the initial PR.

Extra Reading

https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow