Skip to content
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

Render more config fields (#960) #1033

Merged
merged 8 commits into from
Oct 8, 2018

Conversation

beckjake
Copy link
Contributor

Implement #960. Almost all fields in project/profile configs are now rendered.

Unrendered fields are 'on-run-end'/'on-run-start' in the root of a project config, and 'post-hook'/'pre-hook' under 'models' and 'seeds' in a project config.

Profile fields are rendered in a two-stage process (target, then targeted profile), to allow users to require nonexistent environment/cli variables on non-loaded targets without error.

The big config-side change, beyond the obvious rendering stuff, is having to pass the cli variables around to the various classmethod constructors so they can build a renderer.

@beckjake beckjake changed the title Render more config fields Render more config fields (#960) Sep 28, 2018
@drewbanin drewbanin self-requested a review October 2, 2018 00:53
Copy link
Member

@cmcarthur cmcarthur left a comment

Choose a reason for hiding this comment

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

there are some unused functions here.

it strikes me that the focal point of this PR is the deep_map function. it's very hard for me to statically evaluate it. the code looks fine to me, but i'd like to see a really comprehensive suite of tests on it.

dbt/config.py Outdated

@staticmethod
def _is_port_path(keypath):
return len(keypath) == 2 and keypath[-1] == 'port'
Copy link
Member

Choose a reason for hiding this comment

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

this isn't used

dbt/config.py Outdated
if len(keypath) != 4:
return value

if keypath[-1] == 'port' and keypath[1] == 'outputs':
Copy link
Member

Choose a reason for hiding this comment

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

i assume you meant to use _is_port_path here.

this is ok for now. thought: this should be on the adapter as part of "Adapters as Plugins"

dbt/config.py Outdated
return len(keypath) == 2 and keypath[-1] == 'port'

@staticmethod
def _convert_port(value, keypath):
Copy link
Member

Choose a reason for hiding this comment

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

oh... it looks like this also isn't used?


def _render_profile_data(self, value, keypath):
result = self.render_value(value)
if len(keypath) == 1 and keypath[-1] == 'port':
Copy link
Member

Choose a reason for hiding this comment

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

same comment from _convert_port: someday profile-specific rendering info should go on the adapter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adapters will have to provide their own credentials descriptions when they're plugins so I imagine I'll be forced to deal with this then.

Copy link
Member

@cmcarthur cmcarthur left a comment

Choose a reason for hiding this comment

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

hash tag ship it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants