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

Enable calling a macro in a pre- or post-hook config in properties.yml #10603

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Aug 25, 2024

Resolves #7128

Also reported in #10602 and #9482.

Problem

Because pre- and post-hooks within properties.yml files are not late-rendered for models, seeds, etc., references to {{ this }}, macros, etc. don't work:

properties.yml

seeds:
  - name: seed_name
    config:
      post_hook: "alter table {{ this }} alter column id set not null"
    columns:
      - name: id

models:
  - name: my_model
    config:
      pre_hook:
        - "{{ some_macro() }}"

The example seed post-hook above is just a single value (not a list), whereas the example model pre-hook is a list. Both are valid! The former contains Jinja for the this context variable, and the latter contains a Jinja macro.

Solution

Late-render pre- and post-hooks within properties / schema YAML files (like they are for dbt_project.yml)

🎩 Testing

Added two new tests to handle the difference between a hook that is just a string vs. a list of hooks:

  • TestPrePostModelHooksWithMacros
  • TestPrePostModelHooksListWithMacros

❌ ❌ The two new tests failed (as we hoped) prior to the fix:

=========================== short test summary info ============================
FAILED tests/functional/adapter/hooks/test_model_hooks.py::TestPrePostModelHooksWithMacros::test_pre_and_post_run_hooks
FAILED tests/functional/adapter/hooks/test_model_hooks.py::TestPrePostModelHooksListWithMacros::test_pre_and_post_run_hooks

✅ ✅ And both succeed after the fix in fbe20e9.

I also manually checked both the seeds and models examples listed above.

Checklist

  • I have read the contributing guide and understand what's expected of me.
  • I have run this code in development, and it appears to resolve the stated issue.
  • This PR includes tests
  • This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.).
  • This PR includes type annotations for new and modified functions.

@cla-bot cla-bot bot added the cla:yes label Aug 25, 2024
Copy link

codecov bot commented Aug 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.91%. Comparing base (3c55806) to head (0ea7346).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10603      +/-   ##
==========================================
- Coverage   88.92%   88.91%   -0.02%     
==========================================
  Files         180      180              
  Lines       22760    22762       +2     
==========================================
- Hits        20240    20239       -1     
- Misses       2520     2523       +3     
Flag Coverage Δ
integration 86.23% <100.00%> (-0.02%) ⬇️
unit 62.33% <50.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Unit Tests 62.33% <50.00%> (-0.01%) ⬇️
Integration Tests 86.23% <100.00%> (-0.02%) ⬇️

Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@dbeatty10 dbeatty10 marked this pull request as ready for review August 25, 2024 03:22
@dbeatty10 dbeatty10 requested a review from a team as a code owner August 25, 2024 03:22
@dbeatty10 dbeatty10 added the user docs [docs.getdbt.com] Needs better documentation label Aug 25, 2024
@dbeatty10 dbeatty10 merged commit 2218140 into main Aug 27, 2024
77 of 78 checks passed
@dbeatty10 dbeatty10 deleted the dbeatty/fix-7128 branch August 27, 2024 17:08
github-actions bot pushed a commit that referenced this pull request Aug 27, 2024
…ml` (#10603)

* Tests for calling a macro in a pre- or post-hook config in properties.yml

* Late render pre- and post-hooks configs in properties / schema YAML files

* Changelog entry

(cherry picked from commit 2218140)
dbeatty10 added a commit that referenced this pull request Aug 27, 2024
…ml` (#10603) (#10620)

* Tests for calling a macro in a pre- or post-hook config in properties.yml

* Late render pre- and post-hooks configs in properties / schema YAML files

* Changelog entry

(cherry picked from commit 2218140)

Co-authored-by: Doug Beatty <44704949+dbeatty10@users.noreply.github.com>
mirnawong1 added a commit to dbt-labs/docs.getdbt.com that referenced this pull request Sep 5, 2024
[Preview: Calling a macro in a
hook](https://docs-getdbt-com-git-dbeatty10-patch-2-dbt-labs.vercel.app/docs/build/hooks-operations#calling-a-macro-in-a-hook)

[Preview: pre-hook & post-hook

](https://docs-getdbt-com-git-dbeatty10-patch-2-dbt-labs.vercel.app/reference/resource-configs/pre-hook-post-hook)

## What are you changing in this pull request and why?

We have a couple concrete examples of calling a macro in a hook (like
[here](https://docs.getdbt.com/reference/project-configs/on-run-start-on-run-end#call-a-macro-to-grant-privileges)
and
[here](https://docs.getdbt.com/reference/resource-configs/pre-hook-post-hook#apache-spark-analyze-tables-after-creation)).
But we don't currently have any co-located canonical examples of the
different locations where hooks can be configured:
1. SQL model config
1. `properties.yml` \*
1. `dbt_project.yml`

\* Calling a macro in a hook within a properties YAML file was fixed in
v1.8+ by dbt-labs/dbt-core#10603.

## Other pages to review

Once dbt-labs/dbt-core#7128 is resolved, we
should also review these pages to ensure they include examples of
configs across `dbt_project` YAML files, properties YAML files, and
node-level config in SQL files:
- https://docs.getdbt.com/reference/resource-configs/pre-hook-post-hook
-
https://docs.getdbt.com/reference/model-configs#apply-configurations-to-one-model-only
-
https://docs.getdbt.com/reference/seed-configs#apply-the-schema-configuration-to-one-seed-only

## Additional context

Inspired by dbt-labs/dbt-core#10522

## Checklist
- [x] Review the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md)
so my content adheres to these guidelines.

---------

Co-authored-by: Mirna Wong <89008547+mirnawong1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.8.latest cla:yes user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Calling a macro in a pre- or post-hook
3 participants