deis / builder

Git server and application builder for Deis Workflow
https://deis.com
MIT License
40 stars 41 forks source link

ref(k8s_util): make users opt into DOCKER_BUILD_ARGS #498

Closed bacongobbler closed 7 years ago

bacongobbler commented 7 years ago

injecting build args into the image by default has caused a significant performance penalty for applications not utilizing the build args. Making it an opt-in feature flag via deis config:set DEIS_DOCKER_BUILD_ARGS_ENABLED=1 allows users to inject their environment into their Dockerfiles, but they must opt into that performance penalty.

I feel like there's been enough concerns about turning this feature flag on by default in v2.12.0 that we should consider cutting a v2.12.1 with this fix.

deis-bot commented 7 years ago

@ApsOps, @aledbf and @arschles are potential reviewers of this pull request based on my analysis of git blame information. Thanks @bacongobbler!

chexxor commented 7 years ago

I've heard some projects have hundreds of env vars, which sees significant build time increases. A project like that would never be able to opt-in to the "DOCKER_BUILD_ARGS" feature unless they choose exactly which subset of their env vars to include in the build.

So, another idea is to whitelist the env args to include in the build environment. This could like like DEIS_DOCKER_BUILD_ARGS=ENV_VAR_1:ENV_VAR_2:ENV_VAR_3. This could exist alongside the DEIS_DOCKER_BUILD_ARGS_ENABLED env var proposed in this patch, so my proposal doesn't need to burden the addition of this patch.

bacongobbler commented 7 years ago

@chexxor this is sort of a stop-gap fix for now. Please feel free to implement that if you're feeling motivated to work on that, though!

codecov-io commented 7 years ago

Codecov Report

Merging #498 into master will decrease coverage by 0.28%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #498      +/-   ##
==========================================
- Coverage   56.34%   56.05%   -0.29%     
==========================================
  Files          29       29              
  Lines        1237     1238       +1     
==========================================
- Hits          697      694       -3     
- Misses        502      506       +4     
  Partials       38       38
Impacted Files Coverage Δ
pkg/gitreceive/k8s_util.go 70.83% <100%> (+0.17%) :arrow_up:
pkg/healthsrv/buckets_lister.go 80% <0%> (-10%) :arrow_down:
pkg/sshd/server.go 55% <0%> (-2.23%) :arrow_down:
pkg/healthsrv/circuit_state.go 88.88% <0%> (+11.11%) :arrow_up:

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 20989e4...641f70d. Read the comment docs.