update refs in exposure and semantic model definitions when downstream of a split resource #194
Conversation
dave-connors-3
changed the title
There was a problem hiding this comment.
@dave-connors-3 This is great! I had a couple of nitpicks in references.py around how one might simplify, but those are non-blocking IMHO. This change seems both necessary and sufficient
| self, | ||
| resource: CompiledNode, | ||
| current_change_set: Optional[ChangeSet] = None, |
There was a problem hiding this comment.
Hooray for improved formatting!
| ) | ||
| code = ( | ||
| previous_change.data | ||
| if (previous_change and previous_change.data) | ||
| else model_node.raw_code | ||
| else getattr(node, "raw_code", "") |
There was a problem hiding this comment.
This is such a better way of getting the code. Good thinking!
| for ref in refs: | ||
| package_clause = f"'{ref.package}', " if ref.package else "" | ||
| name_clause = f"'{ref.name}'" | ||
| version_clause = f", v={ref.version}" if ref.version else "" | ||
| str_ref = f"ref({package_clause}{name_clause}{version_clause})" | ||
| if str_ref != ref_to_update: | ||
| str_refs.append(str_ref) | ||
| str_refs.append(new_ref) |
There was a problem hiding this comment.
Am I correct in believing that this code fully specifies each resource ref, and then adds the updated upstream source ref? If so, this seems sufficient. Perhaps it would be valuable to create a function that generates ref strings for resources? Either way, ✅
There was a problem hiding this comment.
@dave-connors-3 Thanks for making some formatting/simplification tweaks. This PR looks both necessary and sufficient ✅ 🚀
| elif isinstance(downstream_node, Exposure) or isinstance(downstream_node, SemanticModel): | ||
| is_exposure = isinstance(downstream_node, Exposure) | ||
| data = self.update_yml_resource_references( | ||
| project_name=project_name, | ||
| upstream_resource_name=upstream_node.name, | ||
| resource=downstream_node, | ||
| ) | ||
| return ResourceChange( | ||
| operation=Operation.Update, | ||
| entity_type=EntityType.Exposure if is_exposure else EntityType.SemanticModel, | ||
| identifier=downstream_node.name, | ||
| path=downstream_project.resolve_file_path(downstream_node), | ||
| data=data, | ||
| ) | ||
| raise Exception("Invalid node type provided to generate_reference_update.") |
may close #191
this is a tricky one!
update_child_refs()to updatedepends_onfields for exposures to take a two-arg ref if downstream of a model being split - worked well!lsfunction until 1.7+ -- we need this for a variety of operations, so i updateddbt-corefor this package to1.7.4dbt ls -s +ordersincludes the semantic models attached to orders which means thatdbt-meshify split test --select +ordersmoves the orders semantic model th##at I added to the new project. This is a) good that we do what dbt-core tells us to do but b) not at all the behavior that I expected. As such, I added a test that is just a pass until we can figure out what the expected behavior is.update 1/16
the reason that the semantic model was being selected and moved was because the model and semantic model had the same name! so this was in fact selected as well. This is expected behavior, but is a bit of a stumbling block if trying to have a semantic model reference a newly split resource.
That said, i believe this is ready to be reviewed -- the string matching on refs is not my cleanest code so very open to suggestions!