Substitute build args from config into parsed Dockerfile before processing deps by nkubala · Pull Request #828 · GoogleContainerTools/skaffold

nkubala · 2018-07-18T00:53:50Z

This PR passes the buildArgs from Docker artifacts in skaffold.yaml to the Docker dependency parser, which substitutes the args into the parsed Dockerfile AST before processing any other commands. This allows args to be substituted correctly when provided as a build arg and implicitly referenced in a Dockerfile, e.g.

skaffold.yaml

build:
  artifacts:
  - imageName: foo/bar
    workspace: .
    docker:
      buildArgs:
        FOO: "bar"

Dockerfile

...
ARG FOO
COPY $FOO.txt /tmp/bar.txt
...

will now copy bar.txt to /tmp/bar.txt.

Fixes #777

…ssing deps

dgageot

LGTM with a few nits

dgageot · 2018-07-18T05:31:40Z

cmd/skaffold/app/cmd/docker/deps.go

+func runDeps(out io.Writer, filename, dockerfile, context string) error {
+	config, err := cmdutil.ParseConfig(filename)
+	if err != nil {
+		return err


nit: wrap the error?

dgageot · 2018-07-18T05:31:47Z

cmd/skaffold/app/cmd/docker/context.go

 func runContext(out io.Writer, filename, context string) error {
+	config, err := cmdutil.ParseConfig(filename)
+	if err != nil {
+		return err


nit: wrap the error?

dgageot · 2018-07-18T05:32:34Z

cmd/skaffold/app/cmd/docker/deps.go

+
+func getBuildArgsForDockerfile(config *config.SkaffoldConfig, dockerfile string) map[string]*string {
+	for _, artifact := range config.Build.Artifacts {
+		if artifact.DockerArtifact != nil && artifact.DockerArtifact.DockerfilePath == dockerfile {


Should we compare normalized paths?

Good call. I also realized this was using the top level skaffold.yaml even if the user provided a relative path to the dockerfile which was breaking some scenarios, so I added logic to infer the path to the skaffold.yaml if it's not provided. PTAL

dgageot · 2018-07-18T05:33:38Z

pkg/skaffold/docker/parse.go

 var RetrieveImage = retrieveImage

-func readDockerfile(workspace, absDockerfilePath string) ([]string, error) {
+func readDockerfile(buildArgs map[string]*string, workspace, absDockerfilePath string) ([]string, error) {


nit: would have added those arguments last

bobcatfish · 2018-07-18T17:49:49Z

Hey @nkubala what do you think about adding a new test case or expanding the existing unit tests to cover this case that was failing? (or maybe it's already covered and i'm missing it?)

…ed dockerfile

nkubala · 2018-07-18T20:38:27Z

@bobcatfish added a test

joseaio · 2020-01-08T10:27:30Z

Any news on this feature?
Any workaround?
example: use ARG as value for ENV variable > use $ENV var

ARG MY_ARG=default_value
ENV MY_ENV=$MY_ARG
ADD ${MYENV}.txt /path/to/file.txt

dgageot · 2020-01-08T10:38:14Z

@joseaio I believe this was fixed in #3439

Substitute build args from config into parsed Dockerfile before proce…

7de8b23

…ssing deps

nkubala requested review from dgageot, dlorenc, r2d4 and viglesiasce as code owners July 18, 2018 00:53

dgageot approved these changes Jul 18, 2018

View reviewed changes

nkubala added 3 commits July 18, 2018 11:42

normalize dockerfile paths, and read skaffold.yaml relative to provid…

fc22cdf

…ed dockerfile

review comments

5d64036

add test

94471b4

nkubala merged commit 6a97251 into GoogleContainerTools:master Jul 18, 2018

nkubala deleted the build_args branch July 18, 2018 20:51

jakehow mentioned this pull request Aug 3, 2018

Fix edge cases in build arg processing #870

Merged

Conversation

nkubala commented Jul 18, 2018

Uh oh!

dgageot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobcatfish commented Jul 18, 2018

Uh oh!

nkubala commented Jul 18, 2018

Uh oh!

joseaio commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgageot commented Jan 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joseaio commented Jan 8, 2020 •

edited

Loading