fix var precedence from cli, add tests #739
Conversation
| @@ -25,7 +25,9 @@ def test__cli_vars_longform(self): | |||
| } | |||
| } | |||
| self.run_dbt(["run", "--vars", yaml.dump(cli_vars)]) | |||
| self.run_dbt(["test"]) | |||
|
|
|||
| # TODO : This change in sensible, but why did it not fail before?? | |||
There was a problem hiding this comment.
did you figure it out? you should probably figure it out and remove this comment, or just remove it hah
There was a problem hiding this comment.
yeah -- this worked incorrectly before. The test should have failed, but the vars were stored in a global somewhere so it looked like they were supplied again. I couldn't quite figure out why that happened, but can dig some more if you feel strongly about it.
It's only an issue since we're invoking dbt as a lib from the integration tests
dbt/context/common.py
Outdated
| local_vars = model.config.get('vars', {}) | ||
|
|
||
| self.local_vars = local_vars.copy() | ||
| self.local_vars.update(overrides) |
There was a problem hiding this comment.
we have merge and deep merge fns in utils, use:
self.local_vars = merge(local_vars, overrides)
instead
| if 'vars' not in self.cfg['models']: | ||
| self.cfg['models']['vars'] = {} | ||
|
|
||
| self.cfg['models']['vars'].update(global_vars) |
There was a problem hiding this comment.
why remove this and add global_vars below?
There was a problem hiding this comment.
Previously, this code updated the cfg['models']['vars'] dict with vars defined on the CLI. These vars served as the "base" variables, and were overridden by other variables defined in the models: section of the dbt_project.yml file. My solution here was to store the cli vars separately, then merge them on top of any dbt_project.yml-based configs in the Var object. Can you think of a better way to do this?
|
I synced up with @cmcarthur to discuss these changes. Got the |
, merging
fixes #710
Big TODO here -- why didn't these tests fail previously? Does this PR change behavior beyond the precedence fix?