-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Ct-581 grants as configs #5230
Ct-581 grants as configs #5230
Changes from all commits
4b46fb6
9b788a4
6d18915
31c0e9f
649c9c5
7459608
266b469
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
kind: Features | ||
body: Grants as Node Configs | ||
time: 2022-05-10T20:49:49.197999-04:00 | ||
custom: | ||
Author: gshank | ||
Issue: "5189" | ||
PR: "5230" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,7 @@ class MergeBehavior(Metadata): | |
Append = 1 | ||
Update = 2 | ||
Clobber = 3 | ||
DictKeyAppend = 4 | ||
|
||
@classmethod | ||
def default_field(cls) -> "MergeBehavior": | ||
|
@@ -124,6 +125,9 @@ def _listify(value: Any) -> List: | |
return [value] | ||
|
||
|
||
# There are two versions of this code. The one here is for config | ||
# objects, the one in _add_config_call in context_config.py is for | ||
# config_call_dict dictionaries. | ||
def _merge_field_value( | ||
merge_behavior: MergeBehavior, | ||
self_value: Any, | ||
|
@@ -141,6 +145,31 @@ def _merge_field_value( | |
value = self_value.copy() | ||
value.update(other_value) | ||
return value | ||
elif merge_behavior == MergeBehavior.DictKeyAppend: | ||
if not isinstance(self_value, dict): | ||
raise InternalException(f"expected dict, got {self_value}") | ||
if not isinstance(other_value, dict): | ||
raise InternalException(f"expected dict, got {other_value}") | ||
new_dict = {} | ||
for key in self_value.keys(): | ||
new_dict[key] = _listify(self_value[key]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
Shows up as expected: Whereas:
Shows up as: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really wish Python hadn't chosen to treat strings as sequences... So yeah, listify in all cases would be better. |
||
for key in other_value.keys(): | ||
extend = False | ||
new_key = key | ||
# This might start with a +, to indicate we should extend the list | ||
# instead of just clobbering it | ||
if new_key.startswith("+"): | ||
new_key = key.lstrip("+") | ||
extend = True | ||
if new_key in new_dict and extend: | ||
# extend the list | ||
value = other_value[key] | ||
new_dict[new_key].extend(_listify(value)) | ||
else: | ||
# clobber the list | ||
new_dict[new_key] = _listify(other_value[key]) | ||
return new_dict | ||
|
||
else: | ||
raise InternalException(f"Got an invalid merge_behavior: {merge_behavior}") | ||
|
||
|
@@ -257,6 +286,7 @@ def same_contents(cls, unrendered: Dict[str, Any], other: Dict[str, Any]) -> boo | |
mergebehavior = { | ||
"append": ["pre-hook", "pre_hook", "post-hook", "post_hook", "tags"], | ||
"update": ["quoting", "column_types", "meta"], | ||
"dict_key_append": ["grants"], | ||
} | ||
|
||
@classmethod | ||
|
@@ -427,6 +457,9 @@ class NodeConfig(NodeAndTestConfig): | |
# sometimes getting the Union order wrong, causing serialization failures. | ||
unique_key: Union[str, List[str], None] = None | ||
on_schema_change: Optional[str] = "ignore" | ||
grants: Dict[str, Any] = field( | ||
default_factory=dict, metadata=MergeBehavior.DictKeyAppend.meta() | ||
) | ||
Comment on lines
+460
to
+462
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related tech debt that we discussed yesterday, which is out of scope for this PR, opened as a new issue: #5236 |
||
|
||
@classmethod | ||
def __pre_deserialize__(cls, data): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason we move this logic from
parsed.py
to here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem we were running into with the partial parsing bug was that the config_call_dict wasn't being saved with the partial parsing manifest. So when a new schema file was added the config was being applied to the existing model node, but without the config_call_dict we lose the config from the SQL file.
There were two options to fix it, 1) save the config_call_dict or 2) every time we add a new schema file reload all of the nodes that might be affected. The second option would complicate partial parsing substantially and felt more inconsistent. The config_call_dict wasn't being saved to start with only because my imagination did not think about this case. I clean it up in the WritableManifest post_serialize method because we don't really want or need it in the manifest artifact.