Add ImageOptions to artifact builder.
#4467
Conversation
There was a problem hiding this comment.
I think this is looking good. We should endeavour to remove global state. As a trial, it would also be worth ading the capability to the buildpacks builder, specifically to set the GOOGLE_GOGCFLAGS and GOOGLE_GOLDFLAGS.
pkg/skaffold/build/options.go
Outdated
| type Configuration int | ||
|
|
||
| const ( | ||
| Release = iota |
There was a problem hiding this comment.
I wonder if we should also include Dev here, to differentiate between build, dev, and debug.
There was a problem hiding this comment.
I've changed this to only have Dev and Debug since I also use this parameter in calculating cache hashcode. So having separate Build type would cause the image built by skaffold build not to be used for next skaffold dev
pkg/skaffold/build/options.go
Outdated
| // FIXME: Tag should be []string but we don't support multiple tags yet | ||
| Tag string // image tag | ||
| Configuration Configuration // build image for release or debug | ||
| Args map[string]*string // additional builder specific args |
There was a problem hiding this comment.
Could we call this BuilderArgs? The rest of these options are not specific around the builder.
(What do you envision this for? How would it be set? How is it different from the Docker artifact-builder's buildArgs?)
There was a problem hiding this comment.
I've renamed it to BuildArgs.
The way I see this being used is in differentiating parameters that could influence build between the different skaffold actions like Dev, Build or Debug. Since skaffold.yaml doesn't have sections for these in case there are different settings required those can be put in this generic map. Whichever builder understands that setting would utilize it. This way since the setting affects the built image it also participates in cache hashcode calculation. WDYT?
There was a problem hiding this comment.
A concrete example could be to push the buildpacks builder image digest to this map so that if that changes it invalidates the cache. That'd solve this TODO:
pkg/skaffold/build/local/local.go
Outdated
| @@ -112,3 +113,11 @@ func (b *Builder) getImageIDForTag(ctx context.Context, tag string) (string, err | |||
| } | |||
| return insp.ID, nil | |||
| } | |||
|
|
|||
| func ToDockerOpts(opts *build.ImageOptions) *docker.BuildOptions { | |||
There was a problem hiding this comment.
This should be done in the docker builder itself.
There was a problem hiding this comment.
Pushing ImageOptions into any of the actual builder implementations causes an import cycle (you had predicted this!). So I've defined ImageOptions as the generic builder-agnostic options object. The different builders define their own options object in their respective adapter package.
pkg/skaffold/build/local/local.go
Outdated
| @@ -86,19 +87,19 @@ func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *lat | |||
|
|
|||
| switch { | |||
| case a.DockerArtifact != nil: | |||
| return b.buildDocker(ctx, out, a, tag) | |||
| return b.buildDocker(ctx, out, a, ToDockerOpts(opts)) | |||
There was a problem hiding this comment.
It feels like this ImageOptions should be passed down into the builders.
There was a problem hiding this comment.
Copied from above
Pushing ImageOptions into any of the actual builder implementations causes an import cycle (you had predicted this!). So I've defined ImageOptions as the generic builder-agnostic options object. The different builders define their own options object in their respective adapter package.
Codecov Report
@@ Coverage Diff @@
## master #4467 +/- ##
==========================================
- Coverage 72.22% 72.18% -0.04%
==========================================
Files 329 336 +7
Lines 12850 13038 +188
==========================================
+ Hits 9281 9412 +131
- Misses 2976 3023 +47
- Partials 593 603 +10
Continue to review full report at Codecov.
|
| ARG GOGCFLAGS | ||
| RUN eval go build "${GOGCFLAGS}" -o /app . | ||
| ARG SKAFFOLD_GO_GCFLAGS | ||
| ARG SKAFFOLD_LD_GCFLAGS |
There was a problem hiding this comment.
This should be SKAFFOLD_GO_LDFLAGS
| @@ -89,6 +90,13 @@ func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *late | |||
| inputs = append(inputs, evaluatedEnv...) | |||
| } | |||
|
|
|||
| // add image options hash | |||
| h, err := opts.Hash() | |||
There was a problem hiding this comment.
Hmm, it would be nice if builders that have no change between build, dev, and debug would not need a rebuild. So buildpacks would need a rebuild between build and dev, but docker wouldn't.
| @@ -1,6 +1,11 @@ | |||
| FROM golang:1.12.9-alpine3.10 as builder | |||
|
|
|||
| # These flag values are provided when building for debug | |||
| ARG SKAFFOLD_GO_GCFLAGS | |||
There was a problem hiding this comment.
I won't die on this hill, but this is really supposed to be the most minimal possible example, so this is kind of straying from that. maybe these can go in another example for debug? I don't care too much though
| @@ -0,0 +1,82 @@ | |||
| /* | |||
| Copyright 2019 The Skaffold Authors | |||
There was a problem hiding this comment.
| Copyright 2019 The Skaffold Authors | |
| Copyright 2020 The Skaffold Authors |
|
Closing in favor of #4606 |
Fixes: #4370
Description
This PR introduces
ImageOptionsto the different builders and currently only partially implements it for docker builder and buildpacks.It recognizes
DevandDebugas build configurations and passes different build args likeGO_GCFLAGSwhen building the artifact for debug vs dev. It also updates hashcode formula for image caching so that images forDevvsDebugare cached separately.User facing changes (remove if N/A)
Dockerfile can expose
ARG SKAFFOLD_GO_GCFLAGSandARG SKAFFOLD_GO_LDFLAGSand expect Skaffold to supply meaningful values.Buildpacks auto set
GOOGLE_GCFLAGSandGOOGLE_LDFLAGS(so far).Note: The artifact cached during
skaffold devwill have a different hashcode thanskaffold debugfor all builders going forward