-
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
Docker: Improvements #2720
Docker: Improvements #2720
Conversation
JS File Size Changes (gzipped)A total of 3 files have changed, with a combined diff of +325 B (+19.2%).
|
@RunDevelopment, just a small remark. I think,
For example, FROM alpine AS os takes 1 step and generates 1 image. sudo docker build .
sudo docker images |
I mean, Oh, there is also a |
You're right, the name isn't very fitting. I've been thinking and the name "instruction" isn't even correct for It might be most fitting to rename the
Also, thanks for looking at this! Reviews really help! |
This changes the usage of "instruction" and "keyword" as discussed. It also adds support for some missing keywords.
I need some more time to answer. |
The first part. Thoughts on the I disagree with this part, because I try to draw parallels between Dockerfile and assembly languages. That’s why I suggest the Docker defines a command as a command-line command. This is evidenced by BNFs. See Some quotes:
Examples.
|
I agree! |
The second part. Thoughts on
Let me explain why I consider # An alias is missing!
FROM python:latest AS and # AS is missing!
FROM python:latest bleeding_edge_python give the following error message: FROM requires either one or three arguments Therefore,
|
It looks like I agree with every existing key except
You are welcome! |
First of all, thank you very much for such a detailed explaination.
Agreed. I thought that
I don't think that's a good idea, purely for practical reasons. First of all, from your writing, it's not clear to me whether you suggest this also for the This The Maybe we could add something like
You gave a very good explanation. I still think that they should be highlighted as keywords for two reasons:
No, it isn't. Line continuations are removed. This is indirectly mentioned in the "Note on whitespace" in the format section. Let's look at the following docker file: FROM debian:buster
RUN echo "\
hello\
world" It will result in the following output:
As you can see, the This actually goes even further with the now deprecated empty continuation lines. The following docker file is equivalent to the above one: FROM debian:buster
RUN echo "\
hello\
world" While the name |
That’s true. To be clear: FROM python:latest AS bleeding_edge_python generates <span class="token instruction command">
<span class="token keyword">FROM</span>
python:latest
<span class="token keyword">AS</span>
bleeding_edge_python
</span> The
Sad, but true :(
Yes, I’ve been thinking about making
Marking of
Yes,
The docs say it is :)
By the way, we can redefine the escape character! A backtick (`) should be added here. A single backslash (or a single tick) escapes a single newline and doesn’t affect anything else. The behaviour is similar to the line continuation escape character in the POSIX shell language:
For example, # escape=`
FROM alpine
COPY `t`
`e`
`s`
`t`
`.`
`t`
`x`
`t . makes Docker search for
Well, this is a good point. It makes backslash/backtick work as an “empty line removing” operator. |
Ahh, my bad. I read "escape character" as "character escape". Yes, while the line continuation character is also the normal escape character, a line continuation is not a character escape. So I think it deserves special highlighting.
I know and I hate it.
Not just there. The entire instruction parsing has to be adjusted. It sounds dramatic but it's really just replacing all But I don't know whether we should actually do that. Doing the simple replacement means that we will get false positives for either escape mode. Rn, we don't support backticks but backslashes are handled correctly. I have never seen backticks being used in the wild and deemed them as unimportant. Their expressed purpose is to help writing docker files for Windows VMs but I haven't found any Windows docker files on Dockerhub that actually use them. Do we have to support them? |
Ah, I see... Now we have two syntaxes:
Well, Microsoft mentions the The work is not worth it. Thank you very much for an interesting discussion! |
Exactly. Twice the work because of a feature nobody uses. Thank you for the review and in-depth analysis! |
This makes a bunch of improvements to the Docker language definition.
The main change is that commands are now parsed as one token. This makes it easier to highlight specific parts of commands (like options) and produces fewer false positives than the previous method. The only downside is that it is not easy to match a command whole so the pattern is a little more complex.
I ran the new Docker grammar against some long and complex Dockerfiles from DockerHub and was not able to find any problems.
Other changes:
AS
keyword ofFROM
is now supported.