New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows the profile directory to be set with an environment var #1049

Merged
merged 3 commits into from Oct 17, 2018

Conversation

Projects
None yet
3 participants
@mikekaminsky
Copy link
Contributor

mikekaminsky commented Oct 9, 2018

Passing the CLI flag (as in dbt run --profiles-dir ~/path/to/profile)
should still work. Similarly, if no DBT_PROFILES_DIR environment
variable is set, DBT will look for profiles in the default location.

Allows the profile directory to be set with an environment var
Passing the CLI flag (as in `dbt run --profiles-dir ~/path/to/profile`)
should still work. Similarly, if no DBT_PROFILES_DIR environment
variable is set, DBT will look for profiles in the default location.
@mikekaminsky

This comment has been minimized.

Copy link
Contributor Author

mikekaminsky commented Oct 9, 2018

👋 hi! First time contributor. Proof of concept PR here. LMK if either you don't want this feature or if there are changes I should make for coding-standards stuff.

Haven't gotten my local dev environment all set up for test-runnin'. Wanted to get first reaction feedback from maintainers first.

@drewbanin drewbanin self-requested a review Oct 12, 2018

@drewbanin

This comment has been minimized.

Copy link
Contributor

drewbanin commented Oct 12, 2018

Thanks for the contribution @mikekaminsky! I'm going to check this out, but an initial scan looks pretty good to me.

@beckjake recently did some work to jinja-ify the dbt_project.yml file.

Do you think it would make sense to add a profiles-dir config to the dbt_project.yml spec? That could look like:

profile: my-profile
profiles-dir: '{{ env_var("DBT_PROFILES_DIR", "~/.dbt/profiles.yml") }}'

I think this approach could be interesting, in that

  1. dbt doesn't currently support core config through env vars, so this env var would be a one-off thing
  2. i like the idea of being able to version control a non-env-var default

@beckjake @mikekaminsky do you two buy that?

@beckjake

This comment has been minimized.

Copy link
Contributor

beckjake commented Oct 12, 2018

I think I would prefer to have an environment variable directly responsible, instead of going through a configuration. But I am strongly in favor of being able to control DBT_PROFILES_DIR via environment variables. I was pretty surprised when I started using dbt and found out that exporting a custom DBT_PROFILES_DIR didn't change anything.

@drewbanin

This comment has been minimized.

Copy link
Contributor

drewbanin commented Oct 12, 2018

ok, cool! Let's do it with the env var :)

if os.environ.get('DBT_PROFILES_DIR') is not None:
PROFILES_DIR = os.environ.get('DBT_PROFILES_DIR')
else:
PROFILES_DIR = DEFAULT_PROFILES_DIR

This comment has been minimized.

@beckjake

beckjake Oct 12, 2018

Contributor

This could just be PROFILES_DIR = os.environ.get('DBT_PROFILES_DIR', DEFAULT_PROFILES_DIR)

This comment has been minimized.

@mikekaminsky

mikekaminsky Oct 12, 2018

Author Contributor

D'oh! Will fix.

@@ -23,6 +24,10 @@
DEFAULT_USE_COLORS = True
DEFAULT_PROFILES_DIR = os.path.join(os.path.expanduser('~'), '.dbt')

if os.environ.get('DBT_PROFILES_DIR') is not None:

This comment has been minimized.

@drewbanin

drewbanin Oct 12, 2018

Contributor

I think the way this code works, the supplied env var won't have the ~ expanded. That means that quoted vars won't be expanded, whereas unquoted vars will be expanded.

$ DBT_PROFILES_DIR="~/.dbt/profiles.yml"
$ echo $DBT_PROFILES_DIR
~/.dbt/profiles.yml

$ DBT_PROFILES_DIR=~/.dbt/profiles.yml
$ echo $DBT_PROFILES_DIR
/Users/drew/.dbt/profiles.yml

In the former case, I think dbt will choke on the tilde. Maybe that's just caveat emptor, and a certain amount of bash knowledge is required to use env vars anyway. Alternatively, we could do something like:

if os.environ.get('DBT_PROFILES_DIR') is not None:
  PROFILES_DIR = os.path.expanduser(os.environ.get('DBT_PROFILES_DIR'))

I don't feel super strongly about this, but wanted to raise the topic.

This comment has been minimized.

@beckjake

beckjake Oct 12, 2018

Contributor

Oh, good point! Combining our two suggestions:
PROFILES_DIR = os.path.expanduser(os.environ.get('DBT_PROFILES_DIR', DEFAULT_PROFILES_DIR))

This comment has been minimized.

@drewbanin

drewbanin Oct 12, 2018

Contributor

^ that's a little funky because we'll be expanding the tilde twice when no env var is present, but I don't think it would be an issue, right?

This comment has been minimized.

@beckjake

beckjake Oct 12, 2018

Contributor

The only way it would be possible to have a problem is if there were a posix system where the home directory started with a literal ~, in which case the user will have bigger problems.

@drewbanin

This comment has been minimized.

Copy link
Contributor

drewbanin commented Oct 12, 2018

This closes #1055

Meta comment: I'm going to create retroactive issues for PRs like this (or rather, ask that contributors create issues to describe their eventual PRs) to help bring everything together for a release. It's been difficult in the past to catch all of the new functionality in a given release, so dropping the issue into the milestone should be helpful!

@mikekaminsky

This comment has been minimized.

Copy link
Contributor Author

mikekaminsky commented Oct 12, 2018

@drewbanin @beckjake thanks for the suggestions, y'all. I've implemented that change.

@drewbanin

This comment has been minimized.

Copy link
Contributor

drewbanin commented Oct 12, 2018

Thanks! I just kicked off the full CI build for this PR. I think we'll be good to merge it when the tests pass

@drewbanin

This comment has been minimized.

Copy link
Contributor

drewbanin commented Oct 12, 2018

@mikekaminsky looks like there were some pep8 (formatting) issues here. dbt's unit tests will catch this, so I'd recommend getting at least the unit tests passing locally, then pushing back to this PR.

Drop me a line on Slack if you need help setting up the dev env :)

@mikekaminsky

This comment has been minimized.

Copy link
Contributor Author

mikekaminsky commented Oct 13, 2018

@drewbanin local tests pass :)

@drewbanin drewbanin merged commit 6d66ab0 into fishtown-analytics:dev/guion-bluford Oct 17, 2018

10 checks passed

ci/circleci: integration-bigquery-py27 Your tests passed on CircleCI!
Details
ci/circleci: integration-bigquery-py36 Your tests passed on CircleCI!
Details
ci/circleci: integration-postgres-py27 Your tests passed on CircleCI!
Details
ci/circleci: integration-postgres-py36 Your tests passed on CircleCI!
Details
ci/circleci: integration-redshift-py27 Your tests passed on CircleCI!
Details
ci/circleci: integration-redshift-py36 Your tests passed on CircleCI!
Details
ci/circleci: integration-snowflake-py27 Your tests passed on CircleCI!
Details
ci/circleci: integration-snowflake-py36 Your tests passed on CircleCI!
Details
ci/circleci: unit Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@drewbanin

This comment has been minimized.

Copy link
Contributor

drewbanin commented Oct 17, 2018

Thanks @mikekaminsky! This is going out in 0.12.0 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment