deis / builder

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

fix(slugbuilder): Dont expose the config as env vars during build #447

Closed kmala closed 7 years ago

kmala commented 7 years ago

fixes #444

deis-bot commented 7 years ago

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

codecov-io commented 7 years ago

Current coverage is 57.13% (diff: 68.42%)

Merging #447 into master will increase coverage by 0.39%

@@             master       #447   diff @@
==========================================
  Files            29         29          
  Lines          1165       1199    +34   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            661        685    +24   
- Misses          468        476     +8   
- Partials         36         38     +2   

Powered by Codecov. Last update 3c110dd...aaa7085

aboyett commented 7 years ago

Looks good to me on the first pass. I'll try testing it tomorrow.

bacongobbler commented 7 years ago

@kmala can you please describe what the PR's intention is here? There's a lack of documentation here on the implementation details here for reviewing.

kmala commented 7 years ago

Heroku doesn't expose the app config as the environment vars and instead stores them in the env_dir which is passed as an arg to the compile https://devcenter.heroku.com/articles/buildpack-api#bin-compile. I had created a secret with the config and mounted it in the env dir.

kmala commented 7 years ago

I did a manual test by priniting the environemnt vars and the files in the /tmp/env dir we are passing to the compile.

bacongobbler commented 7 years ago

Did you happen to test running multiple builds of different applications with this change as well? I'm concerned about concurrent builds tripping up each other, and that we handle proper cleanup of the secrets and all that if the build fails.

I'm sure the happy path works, but I would like to make sure that we handle the fail cases as well.

bacongobbler commented 7 years ago

Looks like line 204 in build.go addresses my concern about proper cleanup and e2e ran this without issue, so code LGTM.