Re-factor list of YAML keys for hooks to late-render

[dbeatty10]

resolves #6411

Description

The documentation here appears accurate. But as a user, it's easy for me to miss the subtle difference of a dash and a hyphen and to notice when/where each is relevant.

Is it just possible to treat both post-hook and post_hook as being synonymous? This pull request hopes the answer is YES!

Example

models/my_simple_model.sql

select 1 as "id"

Run it:

dbt run --select my_simple_model

😄 Happy:
dbt_project.yml

models:
  +post-hook:
    - "select 'This works: {{ this }}'"

☹️ Sad:
dbt_project.yml

models:
  +post_hook: 
    - "select 'But this does not: {{ this }}'"

What this pull request needs

• create a new test (or modify an existing one) to cover the sad case shown above
• verify that test fails as-is
• verify it succeeds after making a fix

Approach to testing

Rather than being exhaustive with testing all the variations across models, snapshots, and tests, I opted for just extending the basic test case for models. This was to minimize code added to tests/functional/hooks/test_model_hooks.py as well as not excessively lengthening the time to run tests. If required, we can revisit this decision.

Checklist

• I have read the contributing guide and understand what's expected of me
• I have signed the CLA
• 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
• I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[dbeatty10]

Manually confirmed this fixes the issue. Would be nice to have a test to cover this case to protect against regressions.

[aranke]

Would be nice to have a test to cover this case to protect against regressions.

If there aren't existing tests you can piggyback off, happy to help you set up new tests.

[dbeatty10]

If there aren't existing tests you can piggyback off, happy to help you set up new tests.

That would be awesome if you could help with the testing part @aranke !

I don't yet know if there are any good existing tests to piggyback from, but the following might help search for candidates:

git grep "pre_hook" tests/
git grep "post_hook" tests/
git grep "pre-hook" tests/
git grep "post-hook" tests/

[jtcohen6]

Nice work on this @dbeatty10! Much better than just updating the docs :)

After a few minutes of quick looking, I wasn't able to find a good test for piggy-backing. There's one test I could find with "macro in a p*-hook config defined in dbt_project.yml, but it's a unit test that doesn't do any actual Jinja rendering. I know I always end up opting for functional tests, because they're easiest to write, but that might be appropriate here given the number of different parsing steps that are in the mix for this one.

[dbeatty10]

I've been struggling to think of a simple platform-independent approach to test that post-hook and post_hook are indeed synonymous.

My first thoughts were to add some SQL/DML/etc that modifies the database state, and then we could assert that the intended state is observed. But it's hard to think of a state-changing statement that can be executed in a cross-database manner.

As an alternative approach, what if we try specifying an invalid statement and assert that it raises an error? This would confirm that the hook is being executed regardless if it's specified by post-hook or post_hook -- if it doesn't raise an error, it means that the config didn't take effect (or somehow the statement was allowed by the data platform).

The project configs for the latter approach might look similar to this:

models:
  +post-hook:
    - "This is an invalid statement and should raise an exception"

and

models:
  +post_hook:
    - "This is an invalid statement and should also raise an exception"

[dbeatty10]

I've been struggling to think of a simple platform-independent approach to test that post-hook and post_hook are indeed synonymous.

Turns out it doesn't need to be platform-independent -- guessing can just add/extend relevant functional tests here.

[dbeatty10]

Extended a functional test here and verified that it fails when the bugfix is removed:

=========================== short test summary info ============================
ERROR tests/functional/hooks/test_model_hooks.py::TestPrePostModelHooksUnderscores::test_pre_and_post_run_hooks - dbt.exceptions.CompilationException: Compilation Error

[aranke]

LGTM, maybe we can parameterize these tests, but out of scope for this PR.