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

Add additional warnings for lint rules #4759

Merged
merged 10 commits into from
Mar 29, 2024

Conversation

daghack
Copy link
Collaborator

@daghack daghack commented Mar 13, 2024

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:

  • lowercased stage names
  • lowercased command flag values
  • consistently cased commands

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. 😄 )

@daghack daghack force-pushed the add-lint-warnings branch 3 times, most recently from 25d69b5 to fa21e77 Compare March 13, 2024 22:19
assert.Empty(t, warningList)

df = `FROM scratch AS FOO
ENV FOO bar
Copy link
Member

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;

Perhaps also worth checking for;

Copy link
Member

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☺️

Copy link
Collaborator Author

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

Copy link
Member

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!"

Copy link
Collaborator

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.

@daghack daghack mentioned this pull request Mar 13, 2024
@daghack daghack marked this pull request as draft March 14, 2024 22:51
@daghack daghack force-pushed the add-lint-warnings branch 2 times, most recently from de27657 to fa21e77 Compare March 14, 2024 23:28
Signed-off-by: Talon Bowler <talon.bowler@docker.com>
@daghack
Copy link
Collaborator Author

daghack commented Mar 14, 2024

Screenshot 2024-03-14 at 4 32 11 PM
Screenshot 2024-03-14 at 4 32 44 PM
Screenshot 2024-03-14 at 4 36 10 PM

@daghack
Copy link
Collaborator Author

daghack commented Mar 14, 2024

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 --debug info looking pretty cluttered and having redundant information between the Short and the additional printed information.

@tonistiigi Thoughts/Preferences?

@tonistiigi
Copy link
Member

@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.

frontend/dockerfile/dockerfile2llb/convert.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile2llb/convert_test.go Outdated Show resolved Hide resolved
frontend/dockerfile/linter/ruleset.go Outdated Show resolved Hide resolved
frontend/dockerfile/instructions/parse.go Outdated Show resolved Hide resolved
frontend/dockerfile/instructions/parse.go Outdated Show resolved Hide resolved
frontend/dockerfile/instructions/parse.go Outdated Show resolved Hide resolved
frontend/dockerfile/linter/ruleset.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile2llb/convert.go Outdated Show resolved Hide resolved
frontend/dockerfile/linter/ruleset.go Outdated Show resolved Hide resolved
Signed-off-by: Talon Bowler <talon.bowler@docker.com>
…utput

Signed-off-by: Talon Bowler <talon.bowler@docker.com>
@daghack
Copy link
Collaborator Author

daghack commented Mar 18, 2024

Ended up being a bit of refactoring to get cleaner output, and updated to address @tonistiigi feedback!
Screenshots of updated output (the first being what displays inline during the build, the with the additional context displayed via the docker --debug flag.

Screenshot 2024-03-18 at 8 42 30 AM
Screenshot 2024-03-18 at 8 43 24 AM

Signed-off-by: Talon Bowler <talon.bowler@docker.com>
@daghack daghack marked this pull request as ready for review March 18, 2024 15:58
Signed-off-by: Talon Bowler <talon.bowler@docker.com>
@daghack
Copy link
Collaborator Author

daghack commented Mar 20, 2024

Incorporated lots of feedback by @tonistiigi, now looking for a review! :)

frontend/dockerfile/dockerfile2llb/convert.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile2llb/convert.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile2llb/convert.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile2llb/convert.go Show resolved Hide resolved
frontend/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
Signed-off-by: Talon Bowler <talon.bowler@docker.com>
Signed-off-by: Talon Bowler <talon.bowler@docker.com>
@daghack
Copy link
Collaborator Author

daghack commented Mar 28, 2024

@tonistiigi Ping :)

frontend/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile_test.go Show resolved Hide resolved
frontend/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
frontend/dockerfile/instructions/parse.go Outdated Show resolved Hide resolved
frontend/dockerfile/instructions/parse.go Outdated Show resolved Hide resolved
… update tests

Signed-off-by: Talon Bowler <talon.bowler@docker.com>
Signed-off-by: Talon Bowler <talon.bowler@docker.com>
… instruction casing

Signed-off-by: Talon Bowler <talon.bowler@docker.com>
@daghack
Copy link
Collaborator Author

daghack commented Mar 29, 2024

Getting there, slowly but surely!

@daghack
Copy link
Collaborator Author

daghack commented Mar 29, 2024

@tonistiigi 🎉 No write access, can I get a merge too? 😮 Ty!

@tonistiigi tonistiigi merged commit b3d5726 into moby:master Mar 29, 2024
72 checks passed
@daghack daghack deleted the add-lint-warnings branch March 29, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants