-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 additional warnings for lint rules #4759
Conversation
25d69b5
to
fa21e77
Compare
assert.Empty(t, warningList) | ||
|
||
df = `FROM scratch AS FOO | ||
ENV FOO bar |
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.
Some other possible ideas for linting;
- deprecate Dockerfile legacy 'ENV name value' syntax docker/cli#2743
- relates to dockerfile: allow multiple values for ARG #1692 (comment)
Perhaps also worth checking for;
ARG HTTP_PROXY
and other proxy ones (the _proxy ones are special cases to be always supported)ENV HTTP_PROXY
usually a big "nope" unless you want the image to be only functional in your environment with that proxy.- maybe too strict (?) should we have something to point users to the special "godoc-like" format of comments to document stages? dockerfile: parse comments associated with args and stages #1693
- comments not starting at position 0
- a popular one from the past; [Builder] Warn on empty continuation lines moby#33719
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.
Oh! And if we consider "best practices", we could consider that a Dockerfile should start with a # syntax
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.
Those are great suggestions! There's quite a few linting rules that we want to add moving forward, these three are meant to be just the initial set, with other linting rules being introduced in future PRs! I've updated the initial comment to note that, because this is definitely just a first stepping stone! :D
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.
For sure! I saw your PR and was interested; consider my comment purely as "hey, these might be interesting to add as well!"
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.
These are great, thanks @thaJeztah. I will send out a proposal shortly to capture more input on this.
de27657
to
fa21e77
Compare
Signed-off-by: Talon Bowler <[email protected]>
fa21e77
to
eea0b41
Compare
I'm not a super fan of the lack of any real context in the inline, and think that perhaps having the output in those cases include filename+linenumbers, but when I tried this it came at the cost of the @tonistiigi Thoughts/Preferences? |
@daghack We can improve the short message so that instead of constant text it says "prefix: FOObar casing is inconsistent with the rest of the Dockerfile (app.Dockerfile:12)" or smth similar. |
Signed-off-by: Talon Bowler <[email protected]>
3aa19b8
to
9ea54fa
Compare
…utput Signed-off-by: Talon Bowler <[email protected]>
8554e91
to
e0ad98b
Compare
Ended up being a bit of refactoring to get cleaner output, and updated to address @tonistiigi feedback! |
Signed-off-by: Talon Bowler <[email protected]>
a410543
to
75616a1
Compare
39ba4a0
to
c215f7b
Compare
Signed-off-by: Talon Bowler <[email protected]>
c215f7b
to
793cb68
Compare
Incorporated lots of feedback by @tonistiigi, now looking for a review! :) |
43c597d
to
3f48761
Compare
Signed-off-by: Talon Bowler <[email protected]>
3f48761
to
1e54f28
Compare
Signed-off-by: Talon Bowler <[email protected]>
@tonistiigi Ping :) |
f62229f
to
70ab4ff
Compare
… update tests Signed-off-by: Talon Bowler <[email protected]>
70ab4ff
to
1b966e4
Compare
Signed-off-by: Talon Bowler <[email protected]>
… instruction casing Signed-off-by: Talon Bowler <[email protected]>
Getting there, slowly but surely! |
@tonistiigi 🎉 No write access, can I get a merge too? 😮 Ty! |
We should define a set of rules for evaluating a good or bad Dockerfile. Create a method for evaluating the Dockerfile to ensure obvious issues are highlighted to the user.
This PR adds 3 additional lint warnings for the Dockerfile frontend:
These rules are definitely not the comprehensive list of linting warnings we should add, but instead represent an initial set, with more to come in additional PRs moving forward! 😄
( This PR existed in a similar, previous PR #4751, but only implements the warnings. The additional
--print=lint
functionality will come in its own PR. 😄 )