-
Notifications
You must be signed in to change notification settings - Fork 17
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
Support persisting docs for materialized_view
materializations
#98
Conversation
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR. CLA has not been signed by users: @morsapaes |
Signed the CLA, but...not sure if some manual steps are needed to thread that through? |
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.
@morsapaes thanks! just did some stuff and the tests are running now.
I see that dbt-materialize inherits from dbt-postgres. Does this proposal fix both a postgres issue and materialize issue?
Two things. Can you:
- make a changelog entry with
changie
(docs)? Integration tests are blocked for that reason - open (or link to) an issue on this repo? This is just for tracking purposes?
It fixes a
Opened #120 for tracking purposes, and marked this PR as fixing it. Will push a changelog entry shortly, too. Thanks, @dataders! |
Thanks for the contribution @morsapaes! Do you mind adding a test case for this? We want to at least make sure that a user can submit a materialized view model with a description. Bonus points for verifying that the materialized view has the expected comment. |
I will as soon as I have some bandwidth this week, @mikealfare. |
@morsapaes took a quick pass at some tests, we can open an issue to expand out base adapter persist docs tests to cover materialized views |
The macro that backs object-level
persist_docs
uses{{ relation.type }}
to derive the object type in theCOMMENT ON
statement. This breaks formaterialized_view
materializations:We've fixed this a while back for
dbt-materialize
(which shimsdbt-postgres
) in Materialize #21878, but it really should be fixed at the source!Fixes #120, fixes #11.