Pass env vars defined in Docker build strategy #4562

rhcarvalho · 2015-09-07T16:57:06Z

This does two things:

Changes the Docker strategy builder to insert user-defined environment variables after every FROM instruction of the Dockerfile being built.
Refactors pkg/build/builder based on the Dockerfile manipulation advancements from (1).

Fixes #4470
Trello: https://trello.com/c/DEoNUxVz

0xmichalis · 2015-09-07T18:26:28Z

pkg/build/builder/docker.go

Why not just return dockerfile?

and why not check for this in the place we call insertEnvAfterFrom and just skip calling that function of there are no envs to insert?

and why not check for this in the place we call insertEnvAfterFrom and just skip calling that function of there are no envs to insert?

This may be unexported but it should definitely check for the length of the envs it was given

Good question. The function is supposed to return a new []byte, not the same object in memory.
Otherwise insertEnvAfterFrom([]kapi.EnvVar{}, dockerfile) would behave slightly different than insertEnvAfterFrom([]kapi.EnvVar{{"FOO", "bar"}}, dockerfile).
If later you want to mutate the returned value, in one case you will mutate also the previous value, and in the second case you have two independent byte slices.

Look at how bytes.Join is implemented: http://docs.golang.org/src/bytes/bytes.go?s=7865:7905#L311

@rhcarvalho +1

and why not check for this in the place we call insertEnvAfterFrom and just skip calling that function of there are no envs to insert?

@mfojtik, by itself the function should behave correctly. If you remove the check, insertEnvAfterFrom([]kapi.EnvVar{}, dockerfile) will return something like this:

FROM centos ENV LABEL ... RUN ...

Operating on the different forms the input can take is a natural part of a function definition.

bparees · 2015-09-07T18:29:33Z

@Kargakis had some some work with a dockerfile parsing/validation library. I actually thought it was for manipulating the FROM line, but i guess i'm mistaken..maybe he can point you to it.

0xmichalis · 2015-09-07T18:30:46Z

Nobody likes regexps:)

This also needs to be validated by the docker parser. @rhcarvalho can you refactor and re-use some of #1184 for this?

jhadvig · 2015-09-07T20:33:32Z

pkg/build/builder/docker.go

Dockerfile,

to define the environment variables from env - this half of the the sentence is a bit confusing. Either make it more clear or nuke it.

dockerfile is lowercase because it references the argument name.

to define the environment variables from env - this half of the the sentence is a bit confusing. Either make it more clear or nuke it.

I won't nuke it. Any proposal of a better sentence?
Again, env refers to the argument name. See for example how godocs in the standard library are written: http://docs.golang.org/src/bytes/bytes.go?s=7726:7905#L319. It is common practice to use the argument names in the documentation.

Not saying that its not common, just that the sentence it is a bit confusing. Guess the sentence is just too long, so splitting it would make it more clear.
Or better:
insertEnvAfterFrom inserts an ENV Dockerfile instruction together with environment variables defined in env right after the FROM instruction in the given dockerfile variable.

mfojtik · 2015-09-08T09:32:37Z

@rhcarvalho @Kargakis I wonder if we can just use http://godoc.org/github.com/docker/docker/builder/parser#Node and just inject the FROM and ENV into the pipeline (append our FROM and ENV to the beginning of the parser. In that case we don't have to solve comments, multiple FROM statements etc and we can re-use what Docker already provides... In future we can then build more robust parsers/validators.

rhcarvalho · 2015-09-17T14:36:41Z

@mfojtik @Kargakis @bparees what do you think about this approach of rewriting the Dockerfile?
I have some test code for pkg/util/docker/dockerfile/dockerfile.go but didn't push it yet because it needs some cleaning up.

The new approach relies on the Docker parser heavily 😄

mfojtik · 2015-09-17T14:40:46Z

@rhcarvalho this looks much better, I will review later today. thanks!

rhcarvalho · 2015-09-17T15:00:01Z

I can refactor the existing code to use ParseTreeToDockerfile, FindAll and InsertInstructions in other places...

0xmichalis · 2015-09-17T15:41:53Z

pkg/build/builder/docker.go

Group all origin packages together

FWIW, they're grouped now, just the line comment didn't disappear ;)

0xmichalis · 2015-09-17T16:22:23Z

pkg/util/docker/dockerfile/dockerfile.go

A Find(node *parser.Node, cmd string, index int) that returns the index-th occurence of cmd would also be useful.

When would that be useful?

Possibly you have in mind the replacement of FROM, but better ask...

Scratch specifying a position, I meant returning the position of the last of occurence. Commands like CMD, ENTRYPOINT are valid only once in a Dockerfile and that's the last occurence of them.

s := FindAll(node, command.Cmd) would find all occurrences, then you can decide to consider only the last one: s[len(s)-1]. In this case, your requirement would be better expressed as FindLast(node *parser.Node, cmd string) int. The problem with returning an int is that you want to be able to tell the caller that cmd was not found... I'm having just FindAll for now.

The problem with returning an int is that you want to be able to tell the caller that cmd was not found...

Um, -1 ?

mfojtik · 2015-09-18T12:30:14Z

pkg/util/docker/dockerfile/dockerfile.go

invalid position :-) don't be lazy

pos refers to the argument name. But the message will be better in my next push.... :-)

rhcarvalho · 2015-09-18T22:23:10Z

For those tracking this PR, I've pushed a commit refactoring pkg/build/builder/docker.go: func (d *DockerBuilder) addBuildParameters(dir string) error. Still work in progress, need to fill in some docs, etc. I'm also interested in raising the test coverage of that package which is pretty low ~12% IIRC.

0xmichalis · 2015-09-19T01:11:35Z

@rhcarvalho regarding the new dockerfile package, you might want to have a look at https://github.com/openshift/origin/tree/master/pkg/generate/dockerfile and maybe try to unify these two packages? Follow-up is fine.

rhcarvalho · 2015-09-19T17:40:35Z

regarding the new dockerfile package, you might want to have a look at https://github.com/openshift/origin/tree/master/pkg/generate/dockerfile and maybe try to unify these two

@Kargakis I'll do in a follow up.

rhcarvalho · 2015-09-19T17:43:23Z

I'm done with changes I want to introduce within this PR.
@bparees @liggitt @Kargakis could you please have your final look?

[test]

openshift-bot · 2015-09-19T17:48:43Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5066/)

bparees · 2015-09-19T19:36:20Z

pkg/build/builder/docker.go

document what this "f" function is responsible for doing.

@bparees done.

bparees · 2015-09-19T19:47:43Z

one nit and lgtm.

0xmichalis · 2015-09-20T10:26:35Z

pkg/util/docker/dockerfile/instructions.go

Why do you have this both in here and in pkg/build/builder/common.go? Also would it make sense to use kapi.EnvVar instead?

with kapi.EnvVar comes downward api stuff as well, right? is that something we're able to support for builds?

The three types are conceptually different, and we need to keep the dependencies clear.
WRT kapi.EnvVar and downward API, we'd process references to build fields before calling any function from the pkg/util/docker/dockerfile package.
This util package should know only about Dockerfiles, not about Kubernetes nor downwards API (as it would then have to know about builds as well, and we'd be making a hell of a mess there)...
That's why we convert from kapi.EnvVar to dockerfile.KeyValue, just like before we were already converting kapi.EnvVar into map[string]string used by S2I -- and, as I noted with a comment in the code, using a map is a bad idea. To fix that we'll need to revisit S2I.

openshift-bot · 2015-09-21T07:48:45Z

Evaluated for origin test up to 97eb0bf

bparees · 2015-09-21T14:21:17Z

lgtm. [merge]

openshift-bot · 2015-09-21T14:24:51Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3360/) (Image: devenv-fedora_2376)

openshift-bot · 2015-09-21T14:24:52Z

Evaluated for origin merge up to 97eb0bf

Merged by openshift-bot

0xmichalis reviewed Sep 7, 2015
View reviewed changes

jhadvig reviewed Sep 7, 2015
View reviewed changes

rhcarvalho force-pushed the issue4470-docker-env branch 2 times, most recently from 976e6f5 to 8ecc49c Compare September 17, 2015 14:32

0xmichalis reviewed Sep 17, 2015
View reviewed changes

rhcarvalho force-pushed the issue4470-docker-env branch 2 times, most recently from 0195b33 to ac92ccd Compare September 17, 2015 16:22

0xmichalis reviewed Sep 17, 2015
View reviewed changes

rhcarvalho force-pushed the issue4470-docker-env branch from ac92ccd to b7662e3 Compare September 17, 2015 16:24

mfojtik reviewed Sep 18, 2015
View reviewed changes

rhcarvalho force-pushed the issue4470-docker-env branch 3 times, most recently from e9cae98 to d9640eb Compare September 18, 2015 14:30

rhcarvalho force-pushed the issue4470-docker-env branch 5 times, most recently from 26653f0 to 172b3c2 Compare September 18, 2015 22:21

Pass env vars defined in Docker build strategy

d67b073

rhcarvalho force-pushed the issue4470-docker-env branch 3 times, most recently from 9376474 to 83cf27b Compare September 19, 2015 16:17

rhcarvalho changed the title ~~[WIP] Pass env vars defined in Docker build strategy~~ Pass env vars defined in Docker build strategy Sep 19, 2015

bparees mentioned this pull request Sep 19, 2015

Add example env vars #4735

Merged

bparees reviewed Sep 19, 2015
View reviewed changes

0xmichalis reviewed Sep 20, 2015
View reviewed changes

Refactor pkg/build/builder

97eb0bf

rhcarvalho force-pushed the issue4470-docker-env branch from 83cf27b to 97eb0bf Compare September 21, 2015 07:47

openshift-bot pushed a commit that referenced this pull request Sep 21, 2015

Merge pull request #4562 from rhcarvalho/issue4470-docker-env

dba575d

Merged by openshift-bot

openshift-bot merged commit dba575d into openshift:master Sep 21, 2015

rhcarvalho deleted the issue4470-docker-env branch September 21, 2015 18:07

rhcarvalho mentioned this pull request Sep 29, 2015

Use only official Dockerfile parser #4838

Merged

bparees mentioned this pull request Oct 28, 2015

BuildConfig ENV Variables are not passed to Docker #5408

Closed

Pass env vars defined in Docker build strategy #4562

Pass env vars defined in Docker build strategy #4562

Uh oh!

Conversation

rhcarvalho commented Sep 7, 2015

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!

bparees commented Sep 7, 2015

Uh oh!

0xmichalis commented Sep 7, 2015

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!

mfojtik commented Sep 8, 2015

Uh oh!

rhcarvalho commented Sep 17, 2015

Uh oh!

mfojtik commented Sep 17, 2015

Uh oh!

rhcarvalho commented Sep 17, 2015

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhcarvalho commented Sep 18, 2015

Uh oh!

0xmichalis commented Sep 19, 2015

Uh oh!

rhcarvalho commented Sep 19, 2015

Uh oh!

rhcarvalho commented Sep 19, 2015

Uh oh!

openshift-bot commented Sep 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Sep 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!