Added support for Docker ARG values. #12439

oatmealraisin · 2017-01-10T19:48:02Z

https://trello.com/c/Yh9BPAGi/1051-5-support-dockerfile-args-in-docker-build-techdebt-builds

Through start-build, users are now able to fill ARG values in
Dockerfiles. For more information, see
https://docs.docker.com/engine/reference/builder/#/arg

This adds buildsArgs to the DockerBuildStrategy definition, and
BuildRequest definition.

@csrwng ptal?

oatmealraisin · 2017-01-10T19:48:15Z

[test]

smarterclayton · 2017-01-14T17:52:20Z

pkg/build/api/types.go

@@ -937,6 +941,10 @@ type BuildRequest struct {
 	// Env contains additional environment variables you want to pass into a builder container.
 	Env []kapi.EnvVar

+	// Args contains any build arguments that are to be passed to Docker.


This doesn't belong in this structure unless build args are going to be generic to all strategies.

Roger, it has been removed

@smarterclayton Looking for a different solution, maybe we should have a strategy specific struct in BuildRequest that doesn't always have to be present? Spoke with @csrwng about it for a bit, seems that this may be a good option.

smarterclayton · 2017-01-14T17:52:30Z

pkg/build/api/types.go

@@ -562,6 +562,10 @@ type DockerBuildStrategy struct {
 	// Env contains additional environment variables you want to pass into a builder container
 	Env []kapi.EnvVar

+	// Args contains any build arguments that are to be passed to Docker.  See


Why are Env and BuildArgs distinct?

They are consumed at different parts of the build process. Env variables are put into the build container, the Build Args would be consumed by the docker invocation. Perhaps Env isn't used by Docker Strategies? In that case, we could use Env, but I think that would be a bit confusing in the future.

Env is used already, I don't think we could consolidate BuildArgs and Env together

Is Env used by docker build strategies today?

oatmealraisin · 2017-01-24T15:08:02Z

Putting this on hold until next sprint

oatmealraisin · 2017-02-01T00:27:12Z

@smarterclayton I've changed the BuildRequest to have a BuildStrategyOptions struct, which would allow for different options for different strategy types. ptal?

oatmealraisin · 2017-02-03T15:39:20Z

@smarterclayton ping

oatmealraisin · 2017-02-04T19:19:32Z

Small change to GenericWebhookEvent, changed BuildArgs []kapi.EnvVar to BuildStrategyOptions

bparees · 2017-02-06T15:59:14Z

@openshift/api-review bump please.

oatmealraisin · 2017-02-07T20:31:57Z

@bparees I separated the api changes from the rest of the PR. This should be it's final state, sans your requests of couse ;) ptal?

Removing the WIP tag

bparees

needs tests for new-app/new-build

bparees · 2017-02-07T20:33:38Z

pkg/build/api/types.go

@@ -588,6 +588,11 @@ type DockerBuildStrategy struct {
 	// Env contains additional environment variables you want to pass into a builder container
 	Env []kapi.EnvVar

+	// Args contains any build arguments that are to be passed to Docker.  See
+	// https://docs.docker.com/engine/reference/builder/#/arg for more details
+	// TODO: This should be a docker.BuildArg array, but protobuf has issues w/ it


i think you can remove this TODO. EnvVar serves the purpose just fine.

i see a thumbs up but the line is still here...

bparees · 2017-02-07T20:40:11Z

pkg/build/builder/docker.go

@@ -318,6 +319,13 @@ func (d *DockerBuilder) dockerBuild(dir string, tag string, secrets []api.Secret
 		if d.build.Spec.Strategy.DockerStrategy.DockerfilePath != "" {
 			dockerfilePath = d.build.Spec.Strategy.DockerStrategy.DockerfilePath
 		}
+		// TODO: Check for non-existent build args here, if necessary


Currently, if you pass a build arg to Docker which doesn't get used, the build fails. This is only reported in the Build logs, which may be a bit harder to debug. We could, alternatively, check for build args when we parse the Dockerfile, but this may be more complicated than we want.

I've removed this TODO, but the idea was mentioned elsewhere

yeah i'm not a fan of us taking responsibility for sanity checking such things.

bparees · 2017-02-07T20:41:31Z

pkg/build/builder/docker.go

+		// TODO: Check for non-existent build args here, if necessary
+		if len(d.build.Spec.Strategy.DockerStrategy.BuildArgs) > 0 {
+			envVarBuildArgs := d.build.Spec.Strategy.DockerStrategy.BuildArgs
+			for _, ba := range envVarBuildArgs {


you don't need to do this assignment or the length check. just range over d.build.Spec.Strategy.DockerStrategy.BuildArgs

it won't do anything if it's nil or empty.

bparees · 2017-02-07T20:44:18Z

pkg/build/generator/generator.go

+		result = append(result, kapi.EnvVar{Name: k, Value: v})
+	}
+
+	*oldArgs = result


don't mutate the args, return the new array.

bparees · 2017-02-07T20:44:51Z

pkg/build/generator/generator.go

@@ -240,6 +263,19 @@ func (g *BuildGenerator) Instantiate(ctx kapi.Context, request *buildapi.BuildRe
 		return nil, errors.NewInternalError(err)
 	}

+	// Check if the user specified


specified what?

Changed to better describe the following code

bparees · 2017-02-07T21:00:19Z

pkg/cmd/cli/cmd/startbuild.go

@@ -278,6 +283,16 @@ func (o *StartBuildOptions) Complete(f *clientcmd.Factory, in io.Reader, out, er
 	}
 	o.EnvVar = env

+	// Handle Docker build arguments. In order to leverage existing logic, we
+	// first create an EnvVar array, then convert it to []docker.BuildArg
+	cmdutil.WarnAboutCommaSeparation(o.ErrOut, o.Args, "--build-args")


we don't need to warn about comma separate for this flag because it never supported comma separation before.

bparees · 2017-02-07T21:00:31Z

pkg/cmd/cli/cmd/startbuild.go

+	// Handle Docker build arguments. In order to leverage existing logic, we
+	// first create an EnvVar array, then convert it to []docker.BuildArg
+	cmdutil.WarnAboutCommaSeparation(o.ErrOut, o.Args, "--build-args")
+	// TODO: This prints some stuff that's Env specific, need to change


bparees · 2017-02-07T21:03:54Z

pkg/generate/app/pipeline.go

@@ -90,6 +92,8 @@ func (pb *pipelineBuilder) NewBuildPipeline(from string, input *ImageRef, source
 	}
 	source.Name = name

+	// TODO: We could possibly pre-populate a list of build-args here to prevent
+	// incorrect use


bparees · 2017-02-07T21:06:28Z

test/extended/builds/start.go

+			buildLog, err := br.Logs()
+			o.Expect(err).NotTo(o.HaveOccurred())
+			g.By(fmt.Sprintf("verifying the build failed due to Docker.", br.BuildPath))
+			o.Expect(buildLog).To(o.ContainSubstring("FOO"))


isn't the successful build also going to contain "FOO"? seems like we need a more stringent expected text to check for.

bparees · 2017-02-07T21:06:57Z

test/externals.txt

+https://github.com/gabemontero/forcepull-extended-test-builder.git
+[email protected]:mfojtik/sinatra-app-example
+git://github.com/mfojtik/sinatra-example-2
+git://github.com/mfojtik/sinatra-example-1


I assume you didn't mean to deliver this file...

oatmealraisin · 2017-02-09T21:23:19Z

@bparees Ready for another go, ptal?

bparees · 2017-02-09T21:48:19Z

pkg/build/api/types.go

@@ -588,6 +588,11 @@ type DockerBuildStrategy struct {
 	// Env contains additional environment variables you want to pass into a builder container
 	Env []kapi.EnvVar

+	// Args contains any build arguments that are to be passed to Docker.  See
+	// https://docs.docker.com/engine/reference/builder/#/arg for more details
+	// TODO: This should be a docker.BuildArg array, but protobuf has issues w/ it


i see a thumbs up but the line is still here...

bparees · 2017-02-09T21:56:11Z

pkg/build/api/v1/types.go

+
+	// Args contains any build arguments that are to be passed to Docker.  See
+	// https://docs.docker.com/engine/reference/builder/#/arg for more details
+	// TODO: This should be a docker.BuildArg array, but protobuf has issues w/ it


get rid of the todo.

bparees · 2017-02-09T23:20:59Z

pkg/build/generator/generator.go

+			if newBuild.Spec.Strategy.DockerStrategy != nil {
+				newBuild.Spec.Strategy.DockerStrategy.BuildArgs = updateBuildArgs(&newBuild.Spec.Strategy.DockerStrategy.BuildArgs, dockerOpts.BuildArgs)
+			} else {
+				errors.NewBadRequest(fmt.Sprintf("Cannot specify build args on %s/%s, not a Docker build.", bc.Namespace, bc.ObjectMeta.Name))


don't you need to return this?

bparees · 2017-02-09T23:21:56Z

pkg/build/generator/generator.go

+				newBuild.Spec.Strategy.DockerStrategy.BuildArgs = updateBuildArgs(&newBuild.Spec.Strategy.DockerStrategy.BuildArgs, dockerOpts.BuildArgs)
+			} else {
+				return nil, errors.NewBadRequest(fmt.Sprintf("Cannot specify build args on %s/%s, not a Docker build.", buildConfig.Namespace, buildConfig.ObjectMeta.Name))
+			}


if not_a_docker_build { return nil, errors.blahblah } newbuild.blahblah=updateblahblah()

bparees · 2017-02-09T23:28:40Z

pkg/cmd/cli/cmd/newbuild.go

@@ -95,6 +96,8 @@ type NewBuildOptions struct {
 	Output        string
 	PrintObject   func(obj runtime.Object) error
 	LogsForObject LogsForObjectFunc
+
+	BuildArgs []string


why isn't this part of Config like everything else?

I figured since it was new-build specific, it would be better here

i'd treat it the same way we treat BinaryBuild (which is also specific to new-build).

bparees · 2017-02-09T23:30:07Z

pkg/cmd/cli/cmd/newbuild.go

+	if err != nil {
+		return fmt.Errorf(strings.Replace(err.Error(), "environment variable", "build arg", -1))
+	}
+	o.Config.BuildArgs = buildArgs


i would expect this to be handled in the same place/manner as we handle o.Config.Environment

I didn't add handling of BuildArgs to (AppConfig) validate(), but we are now processing the BuildArgs in buildPipeline, where BuildEnvironment is handled.

bparees · 2017-02-09T23:33:43Z

pkg/cmd/cli/cmd/startbuild.go

+	// first create an EnvVar array, then convert it to []docker.BuildArg
+	buildArgs, _, err := cmdutil.ParseEnv(o.Args, in)
+	if err != nil {
+		return fmt.Errorf(strings.Replace(err.Error(), "environment variable", "build arg", -1))


this is fragile. better to just change ParseEnv to be more generic in its error messages.

bparees · 2017-02-09T23:37:49Z

test/cmd/newapp.sh

@@ -163,6 +163,12 @@ os::cmd::expect_success "oc new-build --binary php --build-env-file /dev/null --
 os::cmd::expect_failure_and_text "echo 'fo%(o=bar' | oc new-build --binary php --build-env-file -" 'invalid parameter assignment'
 os::cmd::expect_failure_and_text "echo 'S P A C E S=test' | oc new-build --binary php --build-env-file -" 'invalid parameter assignment'

+# new-build
+# check that env vars are not split on commas and warning is printed where they previously have


comment is wrong.

oatmealraisin · 2017-02-14T20:02:00Z

@bparees I've updated the PR, ptal?

bparees

@oatmealraisin one last thing and then lgtm. @openshift/api-review bump. this is for 3.6.

bparees · 2017-02-14T20:36:32Z

pkg/build/generator/generator.go

+			}
+			newBuild.Spec.Strategy.DockerStrategy.BuildArgs = updateBuildArgs(&newBuild.Spec.Strategy.DockerStrategy.BuildArgs, dockerOpts.BuildArgs)
+		}
+	}


is the clone path ignoring the request.environment field? it looks like it is. that should be fixed while you're in here.

deads2k · 2017-02-14T21:13:25Z

pkg/build/api/types.go

@@ -936,6 +943,43 @@ type BuildLog struct {
 	unversioned.TypeMeta
 }

+// DockerStrategyOptions contains extra strategy options for Docker builds
+type DockerStrategyOptions struct {
+	unversioned.TypeMeta


Why do these include TypeMeta? Are you using them somewhere as a RawExtension or a runtime.Object directly?

A couple reasons, none of which are important.

./hack/update-generated-deep-copies.sh threw an error when it wasn't there.

I couldn't find a reason why/why not to have them, and I saw that many similar structs had it.

Some structs that are created have no fields, which also caused make update to fail

I've been able to remove it from DockerStrategyOptions and BuildStrategyOptions, but not from the empty stucts.

oatmealraisin · 2017-02-16T21:19:38Z

@deads2k @bparees I need to edit the git history, but all the updated code is there. I removed the unused structs and removed the typemetas. Ptal?

bparees · 2017-02-18T15:18:42Z

@oatmealraisin lgtm, clean up whatever is causing the test failures and squash.

@openshift/api-review need signoff. this is for 1.6.

oatmealraisin · 2017-02-19T00:23:43Z

Squashed. Looks like there is a mix of flakes and stuff that is from the code, am working to resolve

oatmealraisin · 2017-02-21T22:19:00Z

@bparees Tests passing now. I've added a Dockerfile to test/testdata FYI

bparees · 2017-02-21T22:20:20Z

@openshift/api-review bump

smarterclayton · 2017-02-22T02:28:41Z

pkg/build/api/types.go

@@ -911,6 +915,9 @@ type GenericWebHookEvent struct {

 	// Env contains additional environment variables you want to pass into a builder container
 	Env []kapi.EnvVar
+
+	// StrategyOptions contains additional strategy-specific options for the build
+	StrategyOptions *BuildStrategyOptions


I don't like the use of "options" - is there an example from elsewhere that had this? I don't know that this makes sense nested, it's just extra abstraction.

yes, BinaryBuildRequestOptions and BuildLogOptions, in this file.

The nesting has value for options that are common across the strategies (if we had it to do over again, Env would probably go into BuildStrategyOptions), but i'm not going to fight unnesting it into an options field per strategy, as you suggest for the BuildRequest object below.

Then DockerStrategy *DockerStrategyOptions. We don't use Options in any field names that I see and I'm not sure it adds anything.

Let's avoid nesting.

DockerStrategy doesn't sound like a good name for a field that doesn't hold a DockerStrategy object. That is what i'd expect a field with that name to hold.

@smarterclayton bump. i'm open to other naming suggestions but i don't want to just call it DockerStrategy.

smarterclayton · 2017-02-22T02:31:23Z

pkg/build/api/types.go

@@ -966,6 +987,9 @@ type BuildRequest struct {
 	// TriggeredBy describes which triggers started the most recent update to the
 	// buildconfig and contains information about those triggers.
 	TriggeredBy []BuildTriggerCause
+
+	// StrategyOptions contains additional strategy-specific options for the build
+	StrategyOptions *BuildStrategyOptions


This seems better as per strategy options:

DockerStrategy *DockerStrategyOptions SourceStrategy *SourceStrategyOptions

However, this is really starting to fray the design - build requests was never intended to grow to match the build in complexity. The more we allow here the more we distract from Build as the intended unit of submission.

w/o fundamentally changing how start-build works (to have it create a build object instead of a buildrequest object), I don't know how we get there at this point.

It's fine for now.

I've been implementing de-nesting this struct, but now anything that asks for a BuildStrategyOptions would need to ask for a SourceStrategyOptions, DockerStrategyOptions, etc. It's not an issue right now, but I think it will be a bit cumbersome in the future.

Maybe instead of having a containing struct (BuildStrategyOptions), we have an interface that each strategyoption implements?

how would the strategies implement that interface? you'd have to introduce getters and it would have to be the same getters on every implementation, implying they have the same fields.

we can introduce wrapper structs if desired in the future for passing things between methods, they don't have to be part of the api though.

true, sounds good

oatmealraisin · 2017-02-22T19:24:22Z

@smarterclayton @bparees I de-nested DockerStrategyArgs and re-generated everything. ptal? What is the decision on renaming DockerStrategyOptions

bparees · 2017-02-22T19:25:45Z

I still prefer DockerStrategyOptions, or a compromise alternative, over DockerStrategy... so waiting on @smarterclayton to either suggest an alternative or overrule me.

oatmealraisin · 2017-02-24T04:22:20Z

I split the PR into two commits for generated code for now, will squash before merge. This makes it easier for me to re-generate things.

oatmealraisin · 2017-02-28T13:52:08Z

@openshift/api-review bump

smarterclayton · 2017-03-01T16:10:17Z

API approved

bparees · 2017-03-01T17:57:15Z

@oatmealraisin squash

This includes a generic BuildStrategyOptions that will allow us to add additional information to BuildRequest objects, such as BuildArgs for Docker strategies. Added support for Docker ARG values. Through start-build, new-build, and generic webhooks, users are now able to fill ARG values in Dockerfiles. For more information, see https://docs.docker.com/engine/reference/builder/#/arg For start-build, we add the BuildArgs to the DockerStrategyOptions of the BuildRequest. This field was added in an earlier commit to support this. For new-build, we add BuildArgs to the AppConfig, which is then added to a DockerStrategyOptions in buildPipeline(), and is added to the BuildConfig that is created. These BuildArgs will be consumed when the Build is triggered. For webhook, we added a BuildStrategyOptions to the GenericWebhookEvent object. This is consumed by the webhook code, and put into the resulting BuildRequest, similarly to start-build. Internally, the kapi.EnvVar BuildArgs are converted to docker.BuildArg before being consumed by the Docker client. Originally, we used docker.BuildArg throughout the code. However, due to Protobuf complications, it is easier to convert kapi.EnvVar's into docker.BuildArg when we need to.

oatmealraisin · 2017-03-01T18:14:55Z

@bparees Squashed

bparees · 2017-03-01T18:21:53Z

[testextended][extended:core(builds)]

openshift-bot · 2017-03-01T18:30:08Z

Evaluated for origin testextended up to ce319b2

bparees · 2017-03-01T19:48:47Z

flake #12784
flake #11114
[test]

oatmealraisin · 2017-03-01T19:50:31Z

@bparees Thanks

openshift-bot · 2017-03-01T19:54:33Z

Evaluated for origin test up to ce319b2

openshift-bot · 2017-03-01T20:14:09Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1139/) (Base Commit: cb5d93e) (Extended Tests: core(builds))

bparees · 2017-03-01T20:24:19Z

the extended test failures look like all thin pool related issues which @csrwng is working on.
[merge]

openshift-bot · 2017-03-01T20:29:50Z

Evaluated for origin merge up to ce319b2

openshift-bot · 2017-03-01T21:11:58Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/701/) (Base Commit: 0fa7a48)

openshift-bot · 2017-03-03T04:29:30Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/776/) (Base Commit: 79264a2) (Image: devenv-rhel7_6025)

oatmealraisin self-assigned this Jan 10, 2017

oatmealraisin requested a review from csrwng January 10, 2017 19:48

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 11, 2017

smarterclayton added the needs-api-review label Jan 14, 2017

smarterclayton reviewed Jan 14, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2017

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2017

oatmealraisin changed the title ~~WIP Added support for Docker ARG values.~~ Added support for Docker ARG values. Feb 7, 2017

bparees requested changes Feb 7, 2017

View reviewed changes

oatmealraisin mentioned this pull request Feb 7, 2017

Added documentation for adding Docker build args. openshift/openshift-docs#3694

Merged

bparees requested changes Feb 9, 2017

View reviewed changes

bparees requested changes Feb 14, 2017

View reviewed changes

deads2k reviewed Feb 14, 2017

View reviewed changes

bparees added this to the 1.6.0 milestone Feb 18, 2017

smarterclayton reviewed Feb 22, 2017

View reviewed changes

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 24, 2017

smarterclayton added api-approved and removed needs-api-review labels Mar 1, 2017

bparees self-assigned this Mar 1, 2017

openshift-bot merged commit e164e80 into openshift:master Mar 3, 2017

oatmealraisin deleted the rymurphy/docker-args branch March 3, 2017 18:22

Added support for Docker ARG values. #12439

Added support for Docker ARG values. #12439

Conversation

oatmealraisin commented Jan 10, 2017 • edited Loading

oatmealraisin commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Jan 24, 2017

oatmealraisin commented Feb 1, 2017

oatmealraisin commented Feb 3, 2017

oatmealraisin commented Feb 4, 2017

bparees commented Feb 6, 2017

oatmealraisin commented Feb 7, 2017 • edited Loading

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Feb 14, 2017

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Feb 16, 2017

bparees commented Feb 18, 2017

oatmealraisin commented Feb 19, 2017

oatmealraisin commented Feb 21, 2017

bparees commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Feb 22, 2017

bparees commented Feb 22, 2017

oatmealraisin commented Feb 24, 2017

oatmealraisin commented Feb 28, 2017

smarterclayton commented Mar 1, 2017

bparees commented Mar 1, 2017

oatmealraisin commented Mar 1, 2017

bparees commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

oatmealraisin commented Jan 10, 2017 •

edited

Loading

oatmealraisin commented Feb 7, 2017 •

edited

Loading

openshift-bot commented Mar 3, 2017 •

edited

Loading