-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
frontend: command casing rule check after parsing the ast #5243
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.
We need test for this as well that covers #5240
e37249a
to
85681ac
Compare
85681ac
to
2d83878
Compare
Added! |
Dockerfile: dockerfile, | ||
BuildErrLocation: 4, | ||
StreamBuildErr: "failed to solve: dockerfile parse error on line 4: ENV requires at least one argument", | ||
UnmarshalBuildErr: "dockerfile parse error on line 4: ENV requires at least one argument", |
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.
fyi @jedevc
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.
SGTM, but if this is a common typo, potentially there should be a more dedicated lint message.
0b1dbfe
to
64d84e6
Compare
FROM scratch | ||
RUN -<<'EOT' | ||
env | ||
EOT |
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.
Confused about dash in the example
This was a mistake on my hand in the linked ticket, and the -
shouldn't have been there (I blindly wrote it out of habit); see #5240 (comment)
So in this case, there's 3 "commands";
RUN -<<'EOT'
- which will produce an errorenv
- which (at least before this PR) produces both a warning (lowercase) and an errorEOT
- invalid instruction, but it looks like we bail out before reaching that
i.e., this currently (before this PR) produces;
docker build -<<'EOF'
FROM alpine
RUN -<<'EOT'
env
EOT
EOF
[+] Building 0.1s (1/1) FINISHED docker:desktop-linux
=> [internal] load build definition from Dockerfile 0.0s
=> => transferring dockerfile: 70B 0.0s
=> WARN: ConsistentInstructionCasing: Command 'env' should match the case of the command majority (uppercase) (line 3) 0.0s
1 warning found (use docker --debug to expand):
- ConsistentInstructionCasing: Command 'env' should match the case of the command majority (uppercase) (line 3)
Dockerfile:3
--------------------
1 | FROM alpine
2 | RUN -<<'EOT'
3 | >>> env
4 | EOT
5 |
--------------------
ERROR: failed to solve: dockerfile parse error on line 3: ENV requires at least one argument
View build details: docker-desktop://dashboard/build/desktop-linux/desktop-linux/q4mv7723jfvmni1dvj3ptkji2
As the Dockerfile is invalid, to some extent, I'd expect this to;
- skip printing all linting warnings; they're not relevant if the dockerfile is invalid
- if possible, detect (all?) invalid syntax issues (i.e. both
env
as invalid andEOT
being an invalid command) - ☝️ 🤔 that could become noisy though, because the heredoc may contain many lines, all of which would now be considered an "invalid dockerfile instruction"
- 👉 so, yes, perhaps still fail early, but if possible, have a useful "did you mean X?" for the invalid here-doc 🤔
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.
Yeah, I was pretty much just replicating the original failure mode in the tests. That being said, is a fix so that we don't get erroneous rule checks from the contents of the heredoc, and doesn't need to reproduce the dockerfile error in the integration test (which I think the point @tonistiigi was making).
1423095
to
128deb2
Compare
Signed-off-by: Talon Bowler <[email protected]>
128deb2
to
e7779a5
Compare
@tonistiigi Updated the test to not be based on the heredoc typo. :) |
I'm inclined, because I generally think removing checks from the AST stage is a net benefit, even if it's not a fix for the issue where it came up. I'll happily defer to @tonistiigi, though. |
addresses @tonistiigi comments in #5240
Currently we're checking the AST for command violations, but typically it is more convenient and correct for these checks to happen at the instruction level. This updates the consistent command casing check to occur after the AST is parsed out into instructions.