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.

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

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

merged 6 commits into from
Apr 30, 2024

Conversation

stevie-
Copy link
Contributor

@stevie- stevie- commented Apr 10, 2024

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

Fixes #655

How can we test changes

uses existing workflow but tries to run similar steps with the managed container instead of pre install all dependencies.

This helps to

  • give an example how to use the image with github action
  • makes sure we test the container

Signed-off-by: Steffen Tautenhahn <[email protected]>
@stevie- stevie- changed the title feat: use container to run pre-commit feat: Use container to run pre-commit Apr 10, 2024
@MaxymVlasov MaxymVlasov changed the title feat: Use container to run pre-commit docs: Use container to run pre-commit Apr 12, 2024
@MaxymVlasov MaxymVlasov changed the title docs: Use container to run pre-commit docs: Use pre-commit-terraform image to run pre-commit in CI Apr 12, 2024
@MaxymVlasov MaxymVlasov added the estimate/1day Need 1 work day to be done label Apr 12, 2024
stevie- and others added 2 commits April 12, 2024 16:20
README.md Outdated Show resolved Hide resolved

- name: Execute pre-commit
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

@MaxymVlasov MaxymVlasov Apr 15, 2024

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

@MaxymVlasov MaxymVlasov Apr 15, 2024

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.

README.md Outdated
You can use this hook in your GitHub Actions workflow togehther with [pre-commit](https://pre-commit.com). To easy up dependency management, you can use the managed [docker image](#docker-usage) within your workflow. Make sure to set the image tag to the version you want to use.

In this repository's pre-commit [workflow file](.github/workflows/pre-commit.yml) we also check the container image with pre-commit.
Here is another more simple example which includes caching of pre-commit dependencies and uses the `pre-commit` command to run the checks.
Copy link
Collaborator

@MaxymVlasov MaxymVlasov Apr 15, 2024

Choose a reason for hiding this comment

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

Suggested change
Here is another more simple example which includes caching of pre-commit dependencies and uses the `pre-commit` command to run the checks.
Here is another example that includes caching of pre-commit dependencies and uses the `pre-commit` command to run the checks (but fixes will not automatically push back to your branch, when it possible):

Copy link
Collaborator

Choose a reason for hiding this comment

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

@MaxymVlasov : but fixes will be not automatically pushed back to your branch (not sure about the gist of when clause though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see, where the original workflow pushes changes found by pre-commit? Or do you want to be just very specific?

Copy link
Collaborator

@MaxymVlasov MaxymVlasov Apr 15, 2024

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

- name: Execute pre-commit
uses: pre-commit/action@9b88afc9cd57fd75b655d5c71bd38146d07135fe # v2.0.3
env:
SKIP: no-commit-to-branch,hadolint
with:
token: ${{ secrets.GITHUB_TOKEN }}
extra_args: --color=always --show-diff-on-failure --files ${{ steps.file_changes.outputs.files }}
# Run only skipped checks
- name: Execute pre-commit check that have no auto-fixes
if: always()
uses: pre-commit/action@9b88afc9cd57fd75b655d5c71bd38146d07135fe # v2.0.3
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
with:
extra_args: --color=always --show-diff-on-failure --files ${{ steps.file_changes.outputs.files }}

There 2 pre-commit runs.
The first one, is able to push fixes. second-one - not. The difference in availability of GITHUB_TOKEN.

That's done in this way because otherwise, hooks without fixes will produce exit code 1, and that stops from pushing fixed to the branch.

I can say GHA "ignore exit code 1", and fixes will be successfully pushed, but then if something will be detected by hooks that have no autofixes - there will be no notification about that, except inside logs in "successfully done" GHA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed, that you don't use the latest version of the pre-commit/action which has no push support anymore.

So, if the 2nd run doesn't push changes, wouldn't this be a good candidate to be replaced by the container version?

Copy link
Collaborator

@MaxymVlasov MaxymVlasov Apr 16, 2024

Choose a reason for hiding this comment

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

Still no hadolint dependency inside image. It will check nothing from pre-commit-hooks (all of them, which we use here have autofixes or at least do not break the first check when fail) nor pre-commit-terraform repo hooks (as this repo doesn't include any tf code for testing).

If you'd like to make useful tests, check #373.

@MaxymVlasov MaxymVlasov added the documentation Improvements or additions to documentation label Apr 15, 2024
Signed-off-by: Steffen Tautenhahn <[email protected]>
@stevie-
Copy link
Contributor Author

stevie- commented Apr 17, 2024

With my last commit I see this PR as done. What do you think?

README.md Outdated Show resolved Hide resolved
Signed-off-by: Steffen Tautenhahn <[email protected]>
README.md Outdated
* [Authors](#authors)
* [License](#license)
* [Additional information for users from Russia and Belarus](#additional-information-for-users-from-russia-and-belarus)
* [Collection of git hooks for Terraform to be used with pre-commit framework](#collection-of-git-hooks-for-terraform-to-be-used-with-pre-commit-framework)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I get it, we never commit .vscode settings for extensions here...
Maybe that's should be done 🤔

README.md Outdated Show resolved Hide resolved
Co-authored-by: Maksym Vlasov <[email protected]>
@MaxymVlasov MaxymVlasov changed the title docs: Use pre-commit-terraform image to run pre-commit in CI docs: How to use pre-commit-terraform image to run pre-commit in CI Apr 30, 2024
@MaxymVlasov MaxymVlasov merged commit a7697a2 into antonbabenko:master Apr 30, 2024
6 checks passed
@stevie- stevie- deleted the feat/655/add-workflow-with-container-image branch May 2, 2024 07:02
@antonbabenko
Copy link
Owner

This PR is included in version 1.90.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation estimate/1day Need 1 work day to be done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get docker image running in github action
4 participants