Consolidate git actions into commit-check script #10
Conversation
ebeaty-cisco
changed the title
scripts/commit-check
Outdated
| @@ -1,12 +1,72 @@ | |||
| #!/usr/bin/env bash | |||
| # commit-check - Pre-commit checks | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Is this deliberate? Generally safer to pick up /usr/bin/env bash
scripts/commit-check
Outdated
| # correctly incermented, which is done via a seperate github action on the | ||
| # public xrd-helm repository. |
There was a problem hiding this comment.
| # correctly incermented, which is done via a seperate github action on the | |
| # public xrd-helm repository. | |
| # correctly incermented, which is done via a seperate github action. |
Although this is a useful comment I'd prefer to try and keep this repository agnostic of external/internal details/differences.
scripts/commit-check
Outdated
| esac | ||
| done | ||
|
|
||
| if [ "$CONTAINER_TOOL" != "docker" ] && [ "$CONTAINER_TOOL" != "podman" ]; then |
There was a problem hiding this comment.
Is it possible to just avoid this entirely? Apparently the Ubuntu images already have podman installed.
There was a problem hiding this comment.
Yeah, podman works fine in the GitHub action so have removed the option.
scripts/commit-check
Outdated
| echo "Setting up python venv..." | ||
| TMP_VENV=$(mktemp -d -t venv.XXXXXX) | ||
| python3 -m venv "$TMP_VENV" | ||
| # shellcheck source=/dev/null |
There was a problem hiding this comment.
What's this for?
It may be better to explicitly use $TMP_VENV/bin/pip regardless.
There was a problem hiding this comment.
It tells shellcheck to ignore the file being sourced on the next line. Have removed the source and replaced with $TMP_VENV/bin/pip
scripts/commit-check
Outdated
| FAILURES=$((FAILURES+1)) | ||
| fi | ||
|
|
||
| # Build container for UT and run tests using docker or podman | ||
| if [ "$CONTAINER_TOOL" == "docker" ]; then |
There was a problem hiding this comment.
If it's not possible to make this just-podman, then it looks like these two branches are identical, and duplication can just be avoided by just doing e.g., $CONTAINER_TOOL run ...
There was a problem hiding this comment.
Not needed, as I have removed the docker option.
scripts/commit-check
Outdated
| exit 1 | ||
| } | ||
|
|
||
| while getopts ":t:" opt; do |
There was a problem hiding this comment.
We tend to use straight up case statements rather than getopt/getopts - getopts doesn't support long options, but getopt is not standardised.
There was a problem hiding this comment.
No longer needed
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - uses: azure/setup-helm@v3 |
There was a problem hiding this comment.
Do you know if this is needed, or if chart-testing-action pulls in what's needed?
There was a problem hiding this comment.
Not needed, removed.
.github/workflows/release.yml
Outdated
| @@ -1,36 +0,0 @@ | |||
| name: release | |||
There was a problem hiding this comment.
This file shouldn't be deleted - this is for the release job (that automatically creates Helm releases on push to main, not on PR to main).
Arguably we should change this to target main branch only, for the same argument that the version-increment job is targetting main only (AIUI there's no protection against a preview branch overwriting a main branch release if we don't change this).
Move commit checks to a stand alone script so that they can also be ran locally more easily during development and on commit to the internal fork of this repo.
Code changes
commit-checkscript contains the existing commit checks with two caveats:Helm lintis used instead ofct lintfor ease of use outside of git action workflow.Testing
Tests pass when they should.
Also, I have tested with failing tests to ensure that the overall
commit-checkscript fails.