Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs: How to use pre-commit-terraform image to run pre-commit in CI #656

Merged
merged 6 commits into from Apr 30, 2024
Merged

docs: How to use pre-commit-terraform image to run pre-commit in CI #656

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
80ed46b
feat: use container to run pre-commit
stevie- Apr 10, 2024
6d3298c
fix: Apply suggestions from code review
stevie- Apr 12, 2024
407aa20
fix: simpler workflow and Readme
stevie- Apr 12, 2024
ff7bd8b
chore: only document possible workflow
stevie- Apr 16, 2024
ada52e6
chore: use * instead of - for TOC
stevie- Apr 22, 2024
e7835c4
docs: format
stevie- Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 51 additions & 0 deletions .github/workflows/pre-commit.yaml
View file
Open in desktop
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -54,3 +54,54 @@ jobs:
SKIP: check-added-large-files,check-merge-conflict,check-vcs-permalinks,forbid-new-submodules,no-commit-to-branch,end-of-file-fixer,trailing-whitespace,check-yaml,check-merge-conflict,check-executables-have-shebangs,check-case-conflict,mixed-line-ending,detect-aws-credentials,detect-private-key,shfmt,shellcheck
with:
extra_args: --color=always --show-diff-on-failure --files ${{ steps.file_changes.outputs.files }}

pre-commit-container:
runs-on: ubuntu-latest
container:
image: ghcr.io/antonbabenko/pre-commit-terraform:latest
defaults:
run:
shell: bash
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- run: |
git fetch --no-tags --prune --depth=1 origin +refs/heads/*:refs/remotes/origin/*
stevie- marked this conversation as resolved.
Show resolved Hide resolved

- name: Get changed files
id: file_changes
run: |
export DIFF=$(git diff --name-only origin/${{ github.base_ref }} ${{ github.sha }})
echo "Diff between ${{ github.base_ref }} and ${{ github.sha }}"
echo "files=$( echo "$DIFF" | xargs echo )" >> $GITHUB_OUTPUT

- name: fix tar dependency in alpine container image
run: |
apk --no-cache add tar
# check python modules installed versions
python -m pip freeze --local
git config --global --add safe.directory $GITHUB_WORKSPACE
stevie- marked this conversation as resolved.
Show resolved Hide resolved

- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
with:
fetch-depth: 0
ref: ${{ github.event.pull_request.head.sha }}

- name: Cache pre-commit since we use pre-commit from container
uses: actions/cache@v4
with:
path: ~/.cache/pre-commit
key: pre-commit-3|${{ hashFiles('.pre-commit-config.yaml') }}

- name: Execute pre-commit
env:
SKIP: no-commit-to-branch,hadolint
run: |
pre-commit run --color=always --show-diff-on-failure --files ${{ steps.file_changes.outputs.files }}
stevie- marked this conversation as resolved.
Show resolved Hide resolved

# Run only skipped checks
- name: Execute pre-commit check that have no auto-fixes
if: always()
env:
SKIP: check-added-large-files,check-merge-conflict,check-vcs-permalinks,forbid-new-submodules,no-commit-to-branch,end-of-file-fixer,trailing-whitespace,check-yaml,check-merge-conflict,check-executables-have-shebangs,check-case-conflict,mixed-line-ending,detect-aws-credentials,detect-private-key,shfmt,shellcheck
stevie- marked this conversation as resolved.
Show resolved Hide resolved
run: |
pre-commit run --color=always --show-diff-on-failure --files ${{ steps.file_changes.outputs.files }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, before we move forward, I'd like to have the answer to the next question:

Would you like to add a fully functional replacement for the current GH workflow[1], or just show the possibility of docker image usage in GHA[2]?

Btw, [1] little bit related to #373

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[2] - show both ways seems to be a good idea to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[2] - show both ways seems to be a good idea to me.

I guess the .github/workflows/pre-commit.yaml is among active workflows in this repo and hence running the same thing twice might not be desired 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. If just show that it is possible, please:

  1. Update the Readme example with the fixed version from .github/workflows/pre-commit.yaml
  2. Revert changes in .github/workflows/pre-commit.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would mean, that you don't actively check the current container image within GHA.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to your answer to my question above, you not plan to provide a " fully functional replacement for the current GH workflow[1]"

And we definitely won't to decrease the current test coverage. Current realization in this PR will fail on hadolint, shfmt, and shellcheck hooks as there are no such dependencies inside Docker image. (and these checks are vital for .sh and .dockerfile)

And also, it will not push fixes back to branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: The install of additional tools should also work in the container version.