Issue #5871 -- Doc change and unit test to verify docker buildArgs support templates #5884
Conversation
There was a problem hiding this comment.
I don't think Docker artifacts should support an env field: Kaniko does support setting environment variables for the build's pod, but Docker does not propagate environment variables into the container: that's the purpose of the build-args.
Plus v1.EnvVar is a Kubernetes object and shouldn't be included in the docker/GCB build — I opened #5877 to refactor this.
That will simplify this implementation.
| // Env are environment variables passed to the docker build. | ||
| // It also accepts environment variables via the go template syntax. | ||
| // For example: `[{"name": "key1", "value": "value1"}, {"name": "key2", "value": "value2"}, {"name": "key3", "value": "'{{.ENV_VARIABLE}}'"}]`. | ||
| Env []v1.EnvVar `yaml:"env,omitempty"` |
There was a problem hiding this comment.
Docker doesn't propagate environment fields, so this doesn't really make sense to be included here.
There was a problem hiding this comment.
The irony is that there's no need to modify the config for Docker artifacts. At least running Skaffold locally, the file pkg/skaffold/build/gcb/docker.go is not called at all. Instead, most of the work is done in pkg/skaffold/docker/image.go. func (l *localDaemon) Build() calls EvalBuildArgs(), which turns out to be just fine; the missing env parameter isn't really well named, since it's used for non-environmental variables that need to be injected into templates.
I learned this by setting up delve with VS Code, and once I put in a .env file into the top skaffold/ directory of the repo, to my shock the templates took up the environment automatically. master / HEAD is not broken in this respect. But outside of the debugger, broken.
I believe the problem is with how skaffold populates the environment. I can't get .env files to work with skaffold, pretty much no matter where I put them. Do you know how .env files are implemented? I can't find any loader code for the life of me; neither of the two common golang modules used for loading env files is called anywhere, and I could not find any code that was parsing the files and inserting them into the process environment. I'm guessing that VS Code exports the file in code, and then calls fork(), thus passing on the environment pristine. Whatever skaffold is currently doing does not work as well.
There was a problem hiding this comment.
OK, my theory seems sound. I wrote a script to parse my .env file and exported the variable. Do that, and calling skaffold dev resolves my build arg template.
So how can we fix the way that skaffold initializes its environment?
There was a problem hiding this comment.
I added a non-trivial test of the templating feature. The style of the test is a little different from most of the table driven tests used for docker, but was needed to actually exercise EvalBuildArgs(). Short version: the feature works fine in 1.24, and likely earlier than that.
There was a problem hiding this comment.
This PR meandered too much. I've taken the final version and put it in as PR-5901, which should be much easier to understand. Leaving this one open for now so people know the background of it, since this is my first issue in the Skaffold repo.
…ntainerTools#5891) * Use a UUID for the source archive rather than random values
…ogleContainerTools#5890) * move instrumentation prompt funcs into subpackage * refactor instrumentation prompt and output package * linters
torenware
requested review from
chanseokoh,
loosebazooka and
yuwenma
as code owners
Codecov Report
@@ Coverage Diff @@
## master #5884 +/- ##
==========================================
- Coverage 70.91% 70.90% -0.01%
==========================================
Files 447 447
Lines 16908 16909 +1
==========================================
Hits 11990 11990
- Misses 4028 4029 +1
Partials 890 890
Continue to review full report at Codecov.
|
torenware
changed the title
|
Closing — thanks for digging into this @torenware. |
WIP #5871
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs
Description
First pass on the issue. I'd expect that acceptance tests will fail when I create the PR.
I've switched EvalBuildArgs to EvalBuildArgsWithEnv, merging in enough from the Kaniko implementation so that docker.go actually compiles.
Some issues:
make test: a number of docker related tests failed, with errors related to being unable to find Dockerfile. As an experiment, I added a simple Dockerfile to the same directory, and to my surprise, after adding the missed ARG statements, all of the tests but one now pass. FWIW :-) I'd guess there's a mocking mechanism for Dockerfiles which is not being invoked for some reason. Couldn't figure that out.SKAFFOLD_TEST_PACKAGESto the invocation of make, still got a complete set of tests.User facing changes (remove if N/A)
Follow-up Work (remove if N/A)