fix save_test_history macro to work when a model depends_on a source with the same name #106
Conversation
|
|
||
| {% for full_name in node.depends_on.nodes %} | ||
| {% set node_name = full_name.split('.')[-1] %} | ||
| {% if node_name == name %} | ||
| {% set node_type = full_name.split('.')[0] %} |
There was a problem hiding this comment.
Would that work for seed node type?
There was a problem hiding this comment.
Not sure actually, didn't have any seed data in our project so never tested it. Would that come as a separate type here or just as a source or something? I can have a play with seed data if you are not aware and update the PR if needed.
There was a problem hiding this comment.
@akshaykarle Not 100% sure if seeds are a separate type. If you could play with it this would be great, we also have seeds in our tests so those would likely detect that. (We should add the ability to run those on PR's external contributors, as it's a bit hard to test this now)
There was a problem hiding this comment.
thanks, I will test it then and update here in the next few days. and yes, if the tests could run on PR's that would be amazing.
There was a problem hiding this comment.
Got it, will look into running them ;)
There was a problem hiding this comment.
@maciejklimek Maybe something you would like to look into?
There was a problem hiding this comment.
hey @mateuszklimek I finally got some time to test out this issue with seed data. It looks like we cannot have this issue with seed data since we specifically pull only refs and sources based on the regex -
I did also test adding a seed node with the same name as the model but dbt compile itself fails because of the name conflict which didn't happen with source and model so i think we are good on this? Please let me know if there is anything else I can do to help you test and merge this. Would've also been nice to be able to run the integration tests but I suppose that would need more changes which can be a separate PR and I'm not familiar with your CI/CD process.There was a problem hiding this comment.
hey @mateuszklimek this has now been improved and it should work with everything since we pull the node from the existing depends_on list
|
hey @mateuszklimek did you or someone else get a chance to review this? |
…with the same name * Without checking the node_type, a model could match a source with the same name and the macro would error out trying to get the database on that node which is not present for a source * Fixes issue: re-data/re-data#371
…t for model/snapshot types
…depends_on a source with the same name
…safe fix save_test_history macro to work when a model (snapshot or model) depends_on a source with the same name
akshaykarle
force-pushed
the
fix-bug-save-history-failure-371
branch
from
6b04f3f to
4719c98
Compare
|
hey @mateuszklimek is this project still alive? would be good to merge this if possible. Thanks! |
@akshaykarle yes the project is back on track now. I just tested your changes and it looks like they break another fix that was recently merged see #113 I tried adding this line |
What
Describe what the change is solving
By using the separated source and ref lists, we can ensure that we fetch the correct node name for the test.
How
passing the additional
depends_on_nodesparameter topriv_full_name_from_dependsfixes the failing error as it now fetches the correct node for the corresponding source or model