Skip to content
This repository has been archived by the owner on Oct 28, 2022. It is now read-only.

Switch to using a .env file for setting environment variables #107

Merged
merged 1 commit into from Aug 14, 2017

Conversation

wlach
Copy link
Contributor

@wlach wlach commented Aug 4, 2017

This allows us to modify variables for our development instance
(e.g. to use production presto)

@codecov-io
Copy link

codecov-io commented Aug 4, 2017

Codecov Report

Merging #107 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #107   +/-   ##
=======================================
  Coverage   46.29%   46.29%           
=======================================
  Files          13       13           
  Lines         270      270           
  Branches       30       30           
=======================================
  Hits          125      125           
  Misses        143      143           
  Partials        2        2

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 76cf21d...f2357da. Read the comment docs.

Copy link
Contributor

@maurodoglio maurodoglio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a .env file seems to be almost equivalent to using the docker-compose environment directive. Am I missing something?

@wlach
Copy link
Contributor Author

wlach commented Aug 9, 2017

@maurodoglio it's equivalent, it's just easier to modify an .env file without worrying about accidentally committing stuff.

Copy link
Member

@whd whd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach seems fine in that ATMO is doing this, but see comment.

bin/build Outdated
@@ -10,6 +10,8 @@ printf '{"commit":"%s","version":"%s","source":"https://github.com/%s/%s","build
"$CIRCLE_BUILD_URL" \
> version.json

cp .env-dist .env
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be [ ! -f .env ] && cp .env-dist .env. Additionally I don't think this needs to be part of the build step. ATMO has it as an instruction in the README (not ideal because it requires another manual step) and also in bin/test but as I understand it this file is meant to be used by docker-compose and not as an artifact in the container. We should exclude it from being copied to the container if we want to keep it in the build script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whd just to confirm, none of the variables in .env-dist would be applicable for a production deployment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

@maurodoglio
Copy link
Contributor

I noticed that @jezdez wants to move away from .env files in mozilla/telemetry-analysis-service#225. Maybe we should hold on on this?

@wlach
Copy link
Contributor Author

wlach commented Aug 10, 2017

@maurodoglio As @whd pointed out above, we don't actually use the .env file in production so I don't see the issue here. In fact I would think that modifying the docker-compose file would be more of a security risk (since there's a chance of accidentally checking in aws credentials)

@wlach
Copy link
Contributor Author

wlach commented Aug 14, 2017

I removed the changes to bin/build

This allows us to modify variables for our development instance
(e.g. to use production presto)
@wlach wlach merged commit 35ef1d6 into mozilla:master Aug 14, 2017
@wlach wlach deleted the use-env-file branch August 14, 2017 21:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants