-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
feat: Add support for pre-commit/pre-commit-hooks
in Docker image
#374
feat: Add support for pre-commit/pre-commit-hooks
in Docker image
#374
Conversation
Dockerfile
Outdated
@@ -161,7 +161,8 @@ RUN apk add --no-cache \ | |||
# pre-commit deps | |||
git=~2 \ | |||
# All hooks deps | |||
bash=~5 | |||
bash=~5 \ | |||
alpine-sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the comment be added to this line to align with how other packages in this block are described?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean, do you mean should we add another comment to differentiate the hooks deps.
alpine-sdk | |
# Third party hooks deps | |
alpine-sdk |
Like this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's what I mean. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alpine-sdk
is a too big package. pre-commit "default" hooks need https://wiki.alpinelinux.org/wiki/GCC- Depending on what you use, you need different hooks and deps. I oppose adding half of the internet to the Docker image because some folks besides TF can use Golang, Python, Bash, Ruby, YAML, etc. in their IaC.
So, first of all, we need to understand what we'd like to support and how, because what I see in this PR is not really is:
Add alpine-sdk as package to be able to run third party hooks
It was working before with the image v1.62.3, even if it wasn't something that was specifically wanted for this image. The question that I am asking then is could we support at least the hooks from I would understand any answer as I see the pros and cons adding this. We can discuss this further if you would like to. |
I suggest installing as minimum as possible deps. Let's find min req deps and add only them. With major version pining. |
Hi, Thank you for your feedbacks. I update the PR so we only install Tell me if you want me to change the title of the PR and rebase to only have one commit. Thank you ! |
pre-commit/pre-commit-hooks
in Docker image
# [1.70.0](v1.69.0...v1.70.0) (2022-04-28) ### Features * Add support for `pre-commit/pre-commit-hooks` in Docker image ([#374](#374)) ([017da74](017da74))
This PR is included in version 1.70.0 🎉 |
Put an
x
into the box if that apply:Description of your changes
Fixes #359 :
Add
alpine-sdk
package so we are able to run third party checks. This is mostly to be able to run directly this docker image with checks coming fromhttps://github.com/pre-commit/pre-commit-hooks
since they are widely use in combination withpre-commit-terraform
.This is to avoid all the users of the community using pre-commit-terraform to build their own image on top of the one existing.
FIY: This was working before the v1.63.0. So I guess it is a change coming directly from the image of Python.
-->
How can we test changes
.pre-commit-config.yaml
file containing:docker build -t pre-commit-terraform --build-arg INSTALL_ALL=true .
docker run -v $(pwd):/lint -it -w /lint pre-commit-terraform run -a
All the tests should run.