Skip to content
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

Only set the comment when persist_docs.relation is set #344

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Aug 10, 2023

Overview

I accidentally changed default behavior in #317

Resolves #343

Checked that it only sets the comment when:

{{ config(
    ...
    persist_docs={"relation": true}
) }}

And with the following configuration it won't set the comment:

{{ config(
    ...
    persist_docs={"relation": false}
) }}
{{ config(
    ...
) }}

Checklist

  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • README.md updated and added information about my change
  • I have run changie new to create a changelog entry

@hovaesco hovaesco requested a review from damian3031 August 10, 2023 10:12
@Fokko Fokko force-pushed the fd-conditional-comment branch from 609f644 to 2432b48 Compare August 10, 2023 21:37
@Fokko
Copy link
Contributor Author

Fokko commented Aug 17, 2023

Gentle ping @damian3031

dbt/include/trino/macros/adapters.sql Outdated Show resolved Hide resolved
dbt/include/trino/macros/adapters.sql Outdated Show resolved Hide resolved
@damian3031
Copy link
Member

@Fokko I added tests for this change

@damian3031
Copy link
Member

Gentle ping @Fokko 🙂 Please adjust to (or comment on) the requested changes, and PR is good to merge

@Fokko
Copy link
Contributor Author

Fokko commented Sep 15, 2023

@damian3031 Sorry for not replying earlier, thanks for adding tests, and the changes look good 👍🏻

@damian3031
Copy link
Member

@Fokko thanks! Could you squash these 2 new commits into the first one? (this one 2432b48 (#344))

@damian3031 damian3031 force-pushed the fd-conditional-comment branch from 7e75813 to 46b8d0a Compare September 27, 2023 07:15
@damian3031 damian3031 self-requested a review September 27, 2023 07:40
@damian3031 damian3031 merged commit ff99ddc into starburstdata:master Sep 27, 2023
8 checks passed
@damian3031
Copy link
Member

@Fokko thanks for your contribution!

@alaturqua
Copy link

@Fokko Thank you for fixing this.

When is the next release actually?

@damian3031
Copy link
Member

@alaturqua It has just been released in 1.6.2!

@Fokko Fokko deleted the fd-conditional-comment branch October 10, 2023 17:52
@Fokko
Copy link
Contributor Author

Fokko commented Oct 10, 2023

Thanks @damian3031 for shepherding this, and sorry @alaturqua for breaking this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dbt-trino adds comment into table create statement by default
3 participants