Feature/3997 profiles test selection flag #4270
Conversation
There was a problem hiding this comment.
@VersusFacit Thanks for the extra work here! I agree, I think the late bool-ification makes the codepaths much easier to follow, and will make it that much easier when someone comes along to add a third indirect selection mode.
I left one comment. As far as the changelog, feel free to add an rc2 section, and stick this Under the hood, like so. (Assuming that PR gets merged first, you can just pull from main.)
core/dbt/graph/selector_spec.py
Outdated
| indirect_selection = dct.get('indirect_selection', None) or indirect_selection | ||
| if indirect_selection and indirect_selection not in ['eager', 'cautious']: | ||
| raise RuntimeException(f'indirect_selection value "{indirect_selection}" is invalid!') |
There was a problem hiding this comment.
Rather than redefining the acceptable values here, we can use the same enum class for validation:
indirect_selection = IndirectSelection(
dct.get('indirect_selection', None) or indirect_selection
)Then, the error raised is:
17:17:49 | [ error ] | Encountered an error:
'bad_value' is not a valid IndirectSelection
Instead of:
17:14:56 | [ error ] | Encountered an error:
Runtime Error
Could not read selector file data: Runtime Error
indirect_selection value "bad_value" is invalid!
That feels clear enough to me. If you wanted to keep the extra bit of context, we could try catching + wrapping the exception.
There was a problem hiding this comment.
Oh wow. Gonna be honest, I just didn't know the semantics around enums with confidence. Did some local testing and I find it a mental stretch that a StrEnum constructor can take a StrEnum parameter. Not what I was expecting. Suffice to say, I'm onboard with this (even if the underlying class' semantics make me uneasy -- Python quirks strike again 🗯️ ).
|
Addressed your comment and added a CHANGELOG line (also, just the heck of it, cherry-picked in Gerda's hunk in hopes of avoiding merge conflicts 🤞 ) |
* Address 3997. Test selection flag can be in profile.yml. * Per Jerco's 4104 PR unresolved comments, unify i.s. predicate and add env var. * Couple of flake8 touchups. * Classier error handling using enum semantics. * Cherry-pick in part of Gerda's commit to hopefully avoid a future merge conflict. * Add 3997 to changelog. Co-authored-by: Mila Page <[email protected]> automatic commit by git-black, original commits: 5d1b104
* Address 3997. Test selection flag can be in profile.yml. * Per Jerco's 4104 PR unresolved comments, unify i.s. predicate and add env var. * Couple of flake8 touchups. * Classier error handling using enum semantics. * Cherry-pick in part of Gerda's commit to hopefully avoid a future merge conflict. * Add 3997 to changelog. Co-authored-by: Mila Page <[email protected]> automatic commit by git-black, original commits: 5d1b104 e6df426
* Address 3997. Test selection flag can be in profile.yml. * Per Jerco's 4104 PR unresolved comments, unify i.s. predicate and add env var. * Couple of flake8 touchups. * Classier error handling using enum semantics. * Cherry-pick in part of Gerda's commit to hopefully avoid a future merge conflict. * Add 3997 to changelog. Co-authored-by: Mila Page <[email protected]> automatic commit by git-black, original commits: 39f350f 5d1b104
resolves #3997
Description
This is an "extra-credit" roundup of Jerco's comments in PR #4104 . Namely, this unifies the indirect_selection and eagerly_expand predicates. It also uses an enum. The other defining change is the movement towards a flag that can be a DBT_ prefixed env var. In deferring the boolean-ification of this value, it makes the logic quite a bit easier to follow: you're tracing an Enum through the whole thing rather than demanding a sort of pseudo-boolean just for sake of conformity.
All unit and integration tests relevant to this flag are passing locally; includes newly added unit tests and already-present integration tests 066_* from the last PR.
Open concerns:
Checklist
CHANGELOG.mdand added information about my change