Use yaml renderer (with target context) for rendering selectors #5136
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
|
Note: I haven't created tests yet, but wanted to get this out there for inspection and running tests. |
|
Note: this will be backported, but without the test commits. |
There was a problem hiding this comment.
This PR feels like a lot of changes for something we ultimately need to backport to both 1.1.latest + 1.0.latest. After looking through it, most of the changes are:
- Converting relevant test cases + adding new ones — resolving [CT-378] Create tests for vars in packages.yml and selectors.yml #4887, as well as securing the regression case
- Relevant only to rendering in
selectors.yml, which significantly limits the "blast radius" of this change
So, after looking through it, and a bit of local testing, I'm much more comfortable with the change. I'll leave final code review to the Language team. We should aim to merge this ASAP so we can backport and include it in v1.0.6-rc1.
| @@ -114,12 +114,10 @@ def __init__( | |||
| def name(self): | |||
| "Project config" | |||
|
|
|||
| # Uses SecretRenderer | |||
| if cli_vars is None: | ||
| cli_vars = {} |
There was a problem hiding this comment.
Is this just moving the code from SecretRenderer here?
This is the only part that scares me, insofar as it touches code that's more widely used than just selectors.yml. It's hard to imagine how this could cause issues, though.
There was a problem hiding this comment.
This is to "make mypy happy", because in some places we have an Optional[Dict[str, Any]] and in other places it's not Optional. So without this mypy will complain. We do a similar thing in a number of other places.
There was a problem hiding this comment.
Got it! Thanks for the explanation
| @@ -3,7 +3,7 @@ | |||
| from dbt.clients.yaml_helper import yaml, Loader, Dumper, load_yaml_text # noqa: F401 | |||
| from dbt.dataclass_schema import ValidationError | |||
|
|
|||
| from .renderer import SelectorRenderer | |||
| from .renderer import BaseRenderer | |||
There was a problem hiding this comment.
Makes sense to just use BaseRenderer, since the context available to selectors.yml is the same as every other yaml files
|
Last time I made the separate ticket to create tests and it didn't actually happen. I really didn't want to leave this without tests this time, so I included them (the conversion was ready to go). I'm going to cherry-pick the non-test commit for backports. |
* Use yaml renderer (with target context) for rendering selectors * Changie * Convert cli_vars tests * Add test for var in profiles * Add test for cli vars in packages * Add test for vars in selectors
* Use yaml renderer (with target context) for rendering selectors * Changie * Convert cli_vars tests * Add test for var in profiles * Add test for cli vars in packages * Add test for vars in selectors (cherry picked from commit 1f898c8)
…dering selectors (#5161) * Use yaml renderer (with target context) for rendering selectors (#5136) * Use yaml renderer (with target context) for rendering selectors * Changie * Convert cli_vars tests * Add test for var in profiles * Add test for cli vars in packages * Add test for vars in selectors (cherry picked from commit 1f898c8) * Tweak cli_vars_in_packages test to do a run and build an adapter Co-authored-by: Gerda Shank <[email protected]>
…labs#5136) * Use yaml renderer (with target context) for rendering selectors * Changie * Convert cli_vars tests * Add test for var in profiles * Add test for cli vars in packages * Add test for vars in selectors
resolves #5131
Description
When we set up "secret" contexts for profiles.yml and packages.yml, we also made the context for rendering selectors.yml into a secret context (which does not include the target), when it should have stayed as a target context.
Checklist
changie newto create a changelog entry