make native env rendering opt-in #2618
Conversation
beckjake
force-pushed
the
fix/native-env-opt-in
branch
from
ffbcc49 to
84bf169
Compare
There was a problem hiding this comment.
I took this for a spin locally. All the new filters work as I expected. The annoying edge cases we previously had to solve with | as_text or triple quoting have magically disappeared.
I'm okay with the breaking change for port, threads, etc. This only started being possible in 0.17.0, right? (#2486) And it will still be possible by way of as_number. I see it as a small burden (and overall net improvement) for the users who set profile configs via Jinja expressions / env vars. We just need to document the hell out of it.
Heroic work on the unit tests.
There was a problem hiding this comment.
Overall this looks really amazing and I'm thrilled with how this turned out! I'll echo Jerco's comment - the tests here are indeed heroic :)
I added a couple of comments/questions - will be happy to be informed that it is late and my eyes are bleary, but otherwise happy to discuss :)
| returns("'True'"), | ||
| ), | ||
| ( | ||
| '''foo: "'{{ true | as_number }}'"''', |
There was a problem hiding this comment.
This one is surprising to me - I'd think that as_number would fail because true is not numeric. I imagine I'm missing some quirk here - just curious what it might be!
There was a problem hiding this comment.
It's because of the quotes. rendering goes down the "".join(...) path because there are multiple "nodes" in the raw render result.
We could definitely make NumberMarker/BoolMarker examine their inputs and error out here if we want. It would potentially be something silly like: try to literal_eval, make sure the result is a number/bool and error out of not, then throw away that result (or store it and expose it to the concatenation function later). It seemed like a lot of complexity of dubious value since things will end up as strings anyway, but I can see why it'd be desirable.
That's all native mode. In text mode, the as_* are all no-ops so they do nothing.
There was a problem hiding this comment.
ok - that's kind of what i figured. The filter in this context is pretty useless anyway given the wrapping quotes outside of the jinja expression. Works for me!
| ), | ||
| ( | ||
| '''foo:''', | ||
| returns('None'), |
There was a problem hiding this comment.
This one is surprising to me - why do we return a string 'None' instead of a literal None here? This test differs from my experiences with exercising this PR on the CLI. When I test either of the following values, I get a literal None back instead of a stringy 'None'
foo:
foo: null
There was a problem hiding this comment.
dbt doesn't ever pass non-str values to get_rendered, so this aspect of the test doesn't reflect what dbt will actually do during rendering (see dbt.config.renderer.BaseRenderer.render_value).
I wanted to make sure we did have the underlying behavior documented in unit tests, at least. There are integration tests that exercise the behavior you're talking about.
There was a problem hiding this comment.
ahhh, that makes total sense too - thanks
Co-authored-by: Drew Banin <[email protected]>
|
/azp run |
|
Windows tests aren't running due to an outage: https://status.dev.azure.com/ I think they should be fine though, nothing here is windows-specific. |
resolves #2612
Description
Add
as_number,as_native, andas_boolfilters, which call literal_eval and (for number/bool) ensure the output type is valid.The default behavior of
get_renderedis to return whatever the input value is, which preserves expected behavior with Relations. Multiple nodes are converted to text. Theas_textfilter still callsstr()on the result.There is a non-obvious breaking change: Port numbers that are set via jinja evaluation (such as via
"{{ var(...) }}"and" {{ env_var(...) }}") require theas_numberoras_nativefilter. Fixing this to behave the way it did in 0.16.x is going to be quite annoying, and hologram limitations make it tricky to allow string inputs for ports on that level: You can't define a field encoder whose json_schema is aoneOf, because hologram assumes that the schema for a given field will have atype.Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.