asbjornenge / Docker.tmbundle

Dockerfile syntax
MIT License
75 stars 27 forks source link

Multistage support for FROM, AS, COPY, and ARG #17

Closed andyneff closed 6 years ago

andyneff commented 6 years ago

PR for #15

asbjornenge commented 6 years ago

LGTM! @princemaple have a sec to take a look?

@andyneff you are added as collaborator, so feel free to merge when you see fit 👍

asbjornenge commented 6 years ago

Thanks @princemaple 👌 I'll let you guys hash it out 😉 👍 //cc @andyneff

andyneff commented 6 years ago

I'm trying to fix the directive inconsistency now. The cause of my inconsistency was "it worked". Not a good excuse, I grant you that.

When I removed the (), it no longer parsed. So I threw it up to "sublime oddity". I tried adding quotes, and that didn't work either (which I now know was because of the back slashes)

I'd like to use the "folded style", but it looks like (using PyYAML at least) this adds a newline at the end of whatever set:

from yaml import load, dump
load(r"""%YAML 1.2
---
test:
  a: >
    {{onbuild_directive}}(COPY)(\s+--from=(\w+))?
""")

{'test': {'a': '{{onbuild_directive}}(COPY)(\\s+--from=(\\w+))?\n'}}

Unfortunately I'm not an aficionado of YAML and I don't know if this is acceptable, or a bug in PyYAML (which I'm guessing sublime uses internally)

So now that I remove the () and add quotes in I get

variables:
  onbuild_directive: (?:(ONBUILD)\s+)?
  onbuild_commands_directive: "{{onbuild_directive}}(ADD|ARG|ENV|EXPOSE|HEALTHCHECK|LABEL|RUN|SHELL|STOPSIGNAL|USER|VOLUME|WORKDIR)"
  nononbuild_commands_directive: MAINTAINER
  runtime_directive: "{{onbuild_directive}}(CMD|ENTRYPOINT)"
  from_directive: (FROM)\s+\w+([:@](\w+))?(\s+(AS)\s+(\w+))?
  copy_directive: ({{onbuild_directive}}(COPY))(\s+--from=(\w+))?

Why did I not use?:

  copy_directive: "{{onbuild_directive}}(COPY)(\\s+--from=(\\w+))?"

Because COPY is actually a little different. In order to give ONBUILD and COPY the keyword.control.dockerfile, I needed to add the extra group.

So are we happier with this version?

princemaple commented 6 years ago

Hi Andy, thanks a lot for digging this deep into it and writing the thorough explanation. I'll try it out soon and get back to you.

princemaple commented 6 years ago

Looking now, sorry about the long delay. I'm in Australia.

princemaple commented 6 years ago

Hey Andy, the changes look good to me. One last thing, \w does not include - and ., if they are allowed, you may want to add them in. (i.e. replace \w+ with [\w.-]+)

andyneff commented 6 years ago

@princemaple, good catch! I went ahead and just made it as permissive as possible. I have a pretty good idea on most of the limits, but they may change, and I'm not actually sure what are valid character for stage names.

Look good?

princemaple commented 6 years ago

Merged and released.