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
@daghack daghack force-pushed the add-lint-warnings branch from fa21e77 to eea0b41 Compare March 14, 2024 23:30
@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
@daghack daghack force-pushed the add-lint-warnings branch from 3aa19b8 to 9ea54fa Compare March 16, 2024 00:26
@daghack daghack force-pushed the add-lint-warnings branch from 8554e91 to e0ad98b Compare March 18, 2024 14:33
@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

@daghack daghack force-pushed the add-lint-warnings branch from a410543 to 75616a1 Compare March 18, 2024 15:52
@daghack daghack marked this pull request as ready for review March 18, 2024 15:58
@daghack daghack force-pushed the add-lint-warnings branch from 39ba4a0 to c215f7b Compare March 19, 2024 23:47
@daghack daghack force-pushed the add-lint-warnings branch from c215f7b to 793cb68 Compare March 20, 2024 00:05
@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
@daghack daghack force-pushed the add-lint-warnings branch from 43c597d to 3f48761 Compare March 22, 2024 22:17
@daghack daghack force-pushed the add-lint-warnings branch from 3f48761 to 1e54f28 Compare March 22, 2024 23:08
@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
@daghack daghack force-pushed the add-lint-warnings branch from f62229f to 70ab4ff Compare March 29, 2024 16:01
@daghack daghack force-pushed the add-lint-warnings branch from 70ab4ff to 1b966e4 Compare March 29, 2024 16:14
@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