Skip to content

Feat(dbt): Add support for on-run-start and on-run-end hooks#4044

Merged
themisvaltinos merged 5 commits intomainfrom
themis/dbt_on_run_start
Mar 31, 2025
Merged

Feat(dbt): Add support for on-run-start and on-run-end hooks#4044
themisvaltinos merged 5 commits intomainfrom
themis/dbt_on_run_start

Conversation

@themisvaltinos
Copy link
Contributor

This update adds support for dbt's on-run-start and on-run-end hooks which are defined in dbt_project.yml:

on-run-start: sql-statement | [sql-statement]  
on-run-end: sql-statement | [sql-statement]  

Internally, these are mapped to SQLMesh's before_all and after_all statements


As part of this update, the schemas variable is also supported, enabling to grant privileges as you would in dbt with a macro:

{% macro grant_usage_to_schemas(schemas, user) %}
  {% for schema in schemas %}
    GRANT USAGE ON SCHEMA {{ schema }} TO {{ user }};
  {% endfor %}
{% endmacro %}

This macro can then be used in on-run-end:

on-run-end:
  - "{{ grant_usage_to_schemas(schemas, 'user') }}"

This is also supported in native SQLMesh projects for before_all and after_all statements:

after_all:
  - "@grant_usage_to_role(@schemas, admin)"

And by defining the corresponding macro:

from sqlmesh import macro

@macro()
def grant_usage_to_role(evaluator, schemas, role):
  if evaluator._environment_naming_info:
    return [
      f"GRANT USAGE ON SCHEMA {schema} TO {role};"
      for schema in schemas
    ]

@themisvaltinos themisvaltinos requested a review from a team March 27, 2025 20:46
Copy link
Collaborator

@erindru erindru left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I'm not 100% on how the JinjaMacroRegistry is meant to be constructed but the tests look like they're exercising the right things

@themisvaltinos themisvaltinos force-pushed the themis/dbt_on_run_start branch from 52da5f5 to 695d0b1 Compare March 28, 2025 23:43
@themisvaltinos
Copy link
Contributor Author

Looks good to me!
I'm not 100% on how the JinjaMacroRegistry is meant to be constructed but the tests look like they're exercising the right things

Thanks for drawing my attention to it! I took a closer look and extended the implementation to also account for multiple projects when constructing and adding the jinja macros to the registry

@themisvaltinos themisvaltinos force-pushed the themis/dbt_on_run_start branch from 695d0b1 to 4a8c2a1 Compare March 31, 2025 08:24
@themisvaltinos themisvaltinos merged commit bb9826a into main Mar 31, 2025
22 checks passed
@themisvaltinos themisvaltinos deleted the themis/dbt_on_run_start branch March 31, 2025 10:54
for project in self._load_projects():
context = project.context.copy()
if manifest := context._manifest:
on_run_start.extend(manifest._on_run_start or [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we access these outside the manifest instance, we shouldn't mark them as private. Or at the very least add a public property method to expose these.

*(gen(stmt) for stmt in statements)
)
jinja_macros = context.jinja_macros
jinja_macros.root_macros = jinja_root_macros
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we do this gymnastics with root macros?

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.

3 participants