Allow environment variables to be used in docker build argument #1912

rahulwa · 2019-04-03T14:13:02Z

Closes #1619 .

googlebot · 2019-04-03T14:13:06Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

rahulwa · 2019-04-03T14:15:30Z

I signed it!

googlebot · 2019-04-03T14:15:51Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

codecov-io · 2019-04-03T14:50:43Z

Codecov Report

Merging #1912 into master will decrease coverage by 0.05%.
The diff coverage is 54.83%.

@@            Coverage Diff             @@
##           master    #1912      +/-   ##
==========================================
- Coverage   56.03%   55.97%   -0.06%     
==========================================
  Files         175      175              
  Lines        7615     7635      +20     
==========================================
+ Hits         4267     4274       +7     
- Misses       2940     2948       +8     
- Partials      408      413       +5

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/build/local/docker.go	`53.19% <0%> (-3.63%)`	⬇️
pkg/skaffold/build/gcb/desc.go	`94.28% <100%> (ø)`	⬆️
pkg/skaffold/docker/parse.go	`78.02% <57.14%> (-1.7%)`	⬇️
pkg/skaffold/docker/image.go	`71.42% <66.66%> (-0.7%)`	⬇️
pkg/skaffold/build/gcb/docker.go	`90.47% <66.66%> (-9.53%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11f8a1b...1db54bd. Read the comment docs.

balopat

Thank you for your first contribution!

please add tests
please add docs to BuildArgs in pkg/skaffold/schema/latest/config.go

pkg/skaffold/docker/parse.go

rahulwa · 2019-04-04T19:25:54Z

@balopat Added test and doc.

balopat

It would be great to have an integration test for this as well - which would have shown that this is not working just yet: you will need to implement the env var expansion in the builder - not just the dependency resolution path: https://github.com/GoogleContainerTools/skaffold/blob/master/pkg/skaffold/docker/image.go#L326

pkg/skaffold/schema/latest/config.go

pkg/skaffold/docker/parse_test.go

balopat · 2019-04-04T21:22:48Z

pkg/skaffold/docker/parse.go

-			value = *buildArgs[key]
+			value, err = evaluateBuildArgsValue(*buildArgs[key])
+			if err != nil {
+				logrus.Errorf("unable to get value for build arg %q", key)


We should fail the build if the template is invalid instead of just logging - please propagate back the err value through expandBuildArgs, readDockerfile up to GetDependencies.

Signed-off-by: Rahul Sinha <[email protected]>

pkg/skaffold/docker/image.go

rahulwa · 2019-04-05T02:57:56Z

I updated an integration test (examples/nodejs as it is a perfect candidate to use environment variable) to have this also, hoping that it passes, unable to run integration tests locally.

Signed-off-by: Rahul Sinha <[email protected]>

rahulwa · 2019-04-05T16:24:24Z

@balopat I guess now it is ready for review.

rahulwa · 2019-04-20T16:48:44Z

I tried to make changes according to master, I hope it's working. Unit tests are working, Can someone please run integration tests and check?

rahulwa · 2019-04-25T17:51:17Z

Any progress on this @nkubala @balopat ?

…o env-dockerarg

rahulwa · 2019-04-26T20:07:09Z

@balopat @nkubala It's been a long time, can I do something to help here?

tejal29

Hi, @rahulwa Sorry for the delay.

All looks good.
few nits

pkg/skaffold/build/gcb/docker_test.go

pkg/skaffold/docker/image_test.go

tejal29 · 2019-04-29T19:07:43Z

I also ran the integration tests for you!

rahulwa · 2019-04-29T20:08:15Z

Thank you very much @tejal29

tejal29 · 2019-04-30T16:28:31Z

integration/examples/nodejs/skaffold.yaml

@@ -4,6 +4,9 @@ build:
  artifacts:
  - image: gcr.io/k8s-skaffold/node-example
    context: backend
+    docker:
+      buildArgs:
+        SCRIPT: "{{.SCRIPT_ENV}}"


i am guessing you meant "{{.SCRIPT}} here?

Thanks for pointing it out.

tejal29 · 2019-04-30T16:29:19Z

@rahulwa Thanks! started a new integration test run. If it passes, we will merge this in today!
Thank you!

comments addressed.

bpetit · 2019-05-10T14:36:05Z

Thank you so much for this PR, but I can't get it work with skaffold v0.29.0 and this configuration:

apiVersion: skaffold/v1beta10
kind: Config
profiles:
  - name: test 
    activation:
      - env: SKAFFOLD_ENV=test
    build:
      artifacts:
        - image: "myimage"
          context: .
          docker:
            dockerfile: Dockerfile
            buildArgs:
              NPM_TOKEN: "{{.NPM_TOKEN}}"

For example with an environment variable like NPM_TOKEN, I get the variable string itself: "{{.NPM_TOKEN}}" in the docker build process.

And using

NPM_TOKEN: {{.NPM_TOKEN}}

Gives:

cannot unmarshal !!map into string

Am I missing something here ?

khrisrichardson · 2019-05-10T15:03:43Z

@bpetit I have the same issue, and noticed that the environment variable build argument is missing from the docker builder in https://skaffold.dev/docs/references/yaml/, but I'm hoping that was just an oversight.

rahulwa · 2019-05-11T07:18:12Z

Indeed, this is not working. Let me check it.
I am really sorry about it, it shouldn’t have occurred.

rahulwa · 2019-05-11T09:10:25Z

@bpetit @khrisrichardson One more setting is required to make it work, useDockerCLI has to true. Something like below works:

build:
  local:
    useDockerCLI: true

@tejal29 @balopat Do we need to update the docs to clarify this condition or it should run regardless of useDockerCLI option?

bpetit · 2019-05-14T09:00:12Z

Thank you for the hint. It does work.
Updating the documentation would be a good thing, in my opinion.

Have a nice day.

rahulwa requested review from balopat, dgageot, nkubala, priyawadhwa and r2d4 as code owners April 3, 2019 14:13

googlebot added the cla: no label Apr 3, 2019

rahulwa force-pushed the env-dockerarg branch from 8eb72bd to f4b33b0 Compare April 3, 2019 14:13

googlebot added cla: yes and removed cla: no labels Apr 3, 2019

rahulwa force-pushed the env-dockerarg branch from f4b33b0 to 305aedb Compare April 3, 2019 14:32

balopat suggested changes Apr 4, 2019

View reviewed changes

pkg/skaffold/docker/parse.go Outdated Show resolved Hide resolved

balopat previously requested changes Apr 4, 2019

View reviewed changes

balopat added the needs-rebase label Apr 4, 2019

rahulwa added 2 commits April 5, 2019 07:19

Allow environment variables to be used in docker build argument

f6338af

Signed-off-by: Rahul Sinha <[email protected]>

Adding test and fixing it

20dd146

Signed-off-by: Rahul Sinha <[email protected]>

rahulwa force-pushed the env-dockerarg branch from 4611aef to 20dd146 Compare April 5, 2019 02:35

rahulwa commented Apr 5, 2019

View reviewed changes

pkg/skaffold/docker/image.go Show resolved Hide resolved

rahulwa force-pushed the env-dockerarg branch from 15ceb59 to 8a910d0 Compare April 5, 2019 03:02

Adding integration test

50d60d1

Signed-off-by: Rahul Sinha <[email protected]>

rahulwa force-pushed the env-dockerarg branch from 8a910d0 to 50d60d1 Compare April 5, 2019 03:02

balopat added the kokoro:run runs the kokoro jobs on a PR label Apr 5, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 5, 2019

balopat added the kokoro:run runs the kokoro jobs on a PR label Apr 6, 2019

rahulwa force-pushed the env-dockerarg branch from 762bf47 to 63984e3 Compare April 18, 2019 20:36

Updating according to master (Removal of plugin system)

48484d5

rahulwa requested a review from tejal29 as a code owner April 20, 2019 16:45

Merge branch 'master' of github.com:GoogleContainerTools/skaffold int…

957083e

…o env-dockerarg

tejal29 reviewed Apr 29, 2019

View reviewed changes

pkg/skaffold/build/gcb/docker_test.go Outdated Show resolved Hide resolved

pkg/skaffold/docker/image_test.go Show resolved Hide resolved

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Apr 29, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 29, 2019

tejal29 self-assigned this Apr 29, 2019

Erroring out properly in tests

dfaad11

rahulwa force-pushed the env-dockerarg branch from d317525 to 2cf4ad4 Compare April 30, 2019 15:33

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Apr 30, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 30, 2019

tejal29 reviewed Apr 30, 2019

View reviewed changes

Generating docs

1db54bd

rahulwa force-pushed the env-dockerarg branch from 2cf4ad4 to 1db54bd Compare April 30, 2019 17:07

tejal29 approved these changes Apr 30, 2019

View reviewed changes

tejal29 merged commit 24de7c3 into GoogleContainerTools:master Apr 30, 2019

reegnz mentioned this pull request Jun 17, 2019

Production vs Development environment variables. #543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow environment variables to be used in docker build argument #1912

Allow environment variables to be used in docker build argument #1912

rahulwa commented Apr 3, 2019

googlebot commented Apr 3, 2019

rahulwa commented Apr 3, 2019

googlebot commented Apr 3, 2019

codecov-io commented Apr 3, 2019 •

edited

Loading

balopat left a comment

rahulwa commented Apr 4, 2019

balopat left a comment

balopat Apr 4, 2019

rahulwa commented Apr 5, 2019

rahulwa commented Apr 5, 2019

rahulwa commented Apr 20, 2019

rahulwa commented Apr 25, 2019 •

edited

Loading

rahulwa commented Apr 26, 2019

tejal29 left a comment

tejal29 commented Apr 29, 2019

rahulwa commented Apr 29, 2019

tejal29 Apr 30, 2019

rahulwa Apr 30, 2019

tejal29 commented Apr 30, 2019

bpetit commented May 10, 2019 •

edited

Loading

khrisrichardson commented May 10, 2019

rahulwa commented May 11, 2019

rahulwa commented May 11, 2019

bpetit commented May 14, 2019

Allow environment variables to be used in docker build argument #1912

Allow environment variables to be used in docker build argument #1912

Conversation

rahulwa commented Apr 3, 2019

googlebot commented Apr 3, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

rahulwa commented Apr 3, 2019

googlebot commented Apr 3, 2019

codecov-io commented Apr 3, 2019 • edited Loading

Codecov Report

balopat left a comment

Choose a reason for hiding this comment

rahulwa commented Apr 4, 2019

balopat left a comment

Choose a reason for hiding this comment

balopat Apr 4, 2019

Choose a reason for hiding this comment

rahulwa commented Apr 5, 2019

rahulwa commented Apr 5, 2019

rahulwa commented Apr 20, 2019

rahulwa commented Apr 25, 2019 • edited Loading

rahulwa commented Apr 26, 2019

tejal29 left a comment

Choose a reason for hiding this comment

tejal29 commented Apr 29, 2019

rahulwa commented Apr 29, 2019

tejal29 Apr 30, 2019

Choose a reason for hiding this comment

rahulwa Apr 30, 2019

Choose a reason for hiding this comment

tejal29 commented Apr 30, 2019

bpetit commented May 10, 2019 • edited Loading

khrisrichardson commented May 10, 2019

rahulwa commented May 11, 2019

rahulwa commented May 11, 2019

bpetit commented May 14, 2019

codecov-io commented Apr 3, 2019 •

edited

Loading

rahulwa commented Apr 25, 2019 •

edited

Loading

bpetit commented May 10, 2019 •

edited

Loading