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

Merge in the on_configuration_change config on relevant pages #5940

Merged
merged 10 commits into from
Aug 27, 2024

Conversation

alison985
Copy link
Contributor

What are you changing in this pull request and why?

  • I added the order in which materializations are run to the materialized.md reference page. It's important and helpful information. I do not know how to make sure the same snippet occurs on multiple doc pages in a DRY way.
  • I added on_configuration_change to the possible model configurations list.
  • I added links to the on_configuration_change page to other pages that may have an interaction with it.

I did not:

  • Attempt to fully integrate on_configuration_change into the doc pages. I linked to it so it's at least on those pages. If you want to edit them more, they are easily gathered for you in this PR. I don't promise it's a complete list.

Questions I think are relevant to include or clarify:

  1. What counts as a configuration change? If it's only for materialized views, then you probably can't mean materialization type changes.
  2. How does on_configuration_change interact with --defer?
  3. When you say "model does not exist in the provided path, do you mean database structure or file structure? Does it understand identifier and alias? If it's database structure and the database object name already exists because it was added manually outside of dbt will it drop/delete that object even though it has no information about it(and hence can't know whether it's allowed to have the power to drop/delete it)?
  4. When you say "when the model exists, but has a different type" do you mean materialization type?
  5. "Determine whether to apply the configuration changes" is both vague and disconcerting. dbt could decide not to follow the on_configuration_change specified by the dbt AE?
  6. How does this interact with grants?
  7. I assume it has no interaction with pre-hooks, post-hooks, on-run-start, on-run-end, correct?
  8. What's the error message and where is it displayed if you specify on_configuration_change and your adapter doesn't support it?

Please go ahead and change it however you want as I'm unlikely to get back to this PR to make any changes myself in a timely manner.

Checklist

  • Review the Content style guide so my content adheres to these guidelines.
  • For docs versioning, review how to version a whole page and version a block of content.
  • Add a checklist item for anything that needs to happen before this PR is merged, such as "needs technical review" or "change base branch."
  • May need technical review as is. Will need technical review if any of the questions posed have answers added in this PR.

@alison985 alison985 requested a review from a team as a code owner August 17, 2024 19:23
Copy link

vercel bot commented Aug 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs-getdbt-com ✅ Ready (Inspect) Visit Preview Aug 27, 2024 3:22pm

@github-actions github-actions bot added content Improvements or additions to content size: small This change will take 1 to 2 days to address labels Aug 17, 2024
Copy link

vercel bot commented Aug 27, 2024

@matthewshaver is attempting to deploy a commit to the dbt-labs Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@matthewshaver matthewshaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @alison985 !

@matthewshaver matthewshaver enabled auto-merge (squash) August 27, 2024 15:17
@matthewshaver matthewshaver merged commit f5c7d27 into dbt-labs:current Aug 27, 2024
3 checks passed
@matthewshaver
Copy link
Contributor

Will take up the additional questions as an issue to address!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Improvements or additions to content size: small This change will take 1 to 2 days to address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants