Skip to content

Fix edge cases in build arg processing#870

Merged
dlorenc merged 1 commit intoGoogleContainerTools:masterfrom
jakehow:build_args_edge_cases
Aug 3, 2018
Merged

Fix edge cases in build arg processing#870
dlorenc merged 1 commit intoGoogleContainerTools:masterfrom
jakehow:build_args_edge_cases

Conversation

@jakehow
Copy link
Copy Markdown
Contributor

@jakehow jakehow commented Aug 3, 2018

With #828 recently merged I tested in some Dockerfiles where we are using build args.

The current implementation misses some functionality and does not handle a couple of edge cases mostly detailed in the documentation here https://docs.docker.com/engine/reference/builder/#arg

  • Build Args support default values: ARG <name>[=<default value>]
  • Docker parses instructions even when there is more than one space between the command and the value. EX: ARG FOO is valid
  • Usage of the build arg can be done with curly braces eg: COPY ${FOO}

There are also two additional issues that I didn't have in my files, but should be handled to match the behavior of docker build... that are not currently handled by this PR

  • Inline defaults: COPY ${FOO:-server.go}

  • Args are scoped to replace values only below where they are declared in the Dockerfile:
    For example if we have a build arg FOO set to start

    FROM ubuntu:14.04 
    COPY ${FOO:-server.go} .
    ARG  FOO
    CMD $FOO
    

    will evaluate to:

    FROM ubuntu:14.04
    COPY server.go .
    ARG  FOO
    CMD start
    

Additional tests have been added for the variations handled by this PR and pass for me.

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@jakehow
Copy link
Copy Markdown
Contributor Author

jakehow commented Aug 3, 2018

@googlebot I've signed the CLA

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@balopat balopat self-assigned this Aug 3, 2018
@dlorenc dlorenc added the kokoro:run runs the kokoro jobs on a PR label Aug 3, 2018
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Aug 3, 2018
Copy link
Copy Markdown
Contributor

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix and detailed test cases!

@dlorenc dlorenc merged commit 24ca814 into GoogleContainerTools:master Aug 3, 2018
@jakehow jakehow deleted the build_args_edge_cases branch August 4, 2018 05:55
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.

5 participants