-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Add Dockerfile linter #6886
🌱 Add Dockerfile linter #6886
Conversation
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.
Overall I like Hadolint, and I like this script - but I don't know how useful it is to introduce into CI because our docker files are more dynamic than Hadolint's static analysis is properly able to deal with.
For example Hadolint marks as invalid, with a warning, our lines like:
FROM ${builder_image} as builder
as invalid as we don't specify an image version. In reality we do set an image version, but in the Makefile.
That said there's currently no results of error type in the repo - just warnings and info - so this linter does work and pass, and I'd expect it to continue to do so.
Overall hadolint is not as useful as it could be, but should help us catch major problems with the Dockerfiles so I'm +1 to the change.
/lgtm
Looking at this again I think it would be possible to fail on warnings and get a more useful linting signal from this byadding the nolint ignore directive ( I'm currently getting:
So once the build-args 3006 issues are no-linted there's only a couple of small changes needed to bring out Dockerfiles in line with what hadolint expects - @oscr WDYT? |
5277aa6
to
ec80136
Compare
@killianmuldoon Excellent suggestions! Thank you for the feedback. It's added now 😄 |
ec80136
to
e862609
Compare
/test pull-cluster-api-e2e-informing-main |
e862609
to
9cca44b
Compare
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.
Awesome work!
/lgtm
Thank you @killianmuldoon for the great feedback! |
/test pull-cluster-api-e2e-informing-main |
9cca44b
to
0c66bcf
Compare
I have updated the linting script to take optional arguments HADOLINT_VER and HADOLINT_FAILURE_THRESHOLD. If these are unspecified (e g running the script directly) they will have default values
|
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.
/lgtm
# Ignore Hadolint rule "Always tag the version of an image explicitly." | ||
# It's an invalid finding since the image is explicitly set in the Makefile. | ||
# https://github.com/hadolint/hadolint/wiki/DL3006 | ||
# hadolint ignore=DL3006 |
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.
Can this be ignored globally instead of having it in each Dockerfile?
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.
@vincepri Yes, it is possible to have global ignore rules. In an earlier review it was requested per file: #6886 (comment) I have no strong opinion here so whatever the community thinks is best I can implement.
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 still think it's more explicit to do it where we need so we catch this if it's changed in e.g. a new Dockerfile.
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 can see that, that's completely fine, it was just noisy when reviewing the PR
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Adds a Dockerfile linter to the make lint target. This is done via a script which finds all files named "Dockerfile" and runs Hadolint on them. I've configured it to only fail on the highest severity.
The new output is:
If you want to verify a failure, here is a failing Dockerfile that you can add:
And verify exitcode with
echo $?