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

possibility to use templated properties to apply by default on file update #38

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

david-whimsical
Copy link

@david-whimsical david-whimsical commented Sep 15, 2023

hey, first of all, thanks for building this package, it's been a pleasure using it!


Our team recently started to use what's in the meta field, most notably to set the meta.owner property:

version: 2
models:
- name: dim_user
  description: >
    Some description
  meta:
    owner: '@data-team'
  columns:
  - name: user_id
    description: ''
    tests:
    - not_null
    - unique
  - name: some_col
    description: ''
  - name: some_other_col
    description: ''

In this case, @data-team is our default, which we sometimes replace with an individual owner, eg. @david, if the data model is very particular. This default value, we want it on every single data models that we create, and it was a little tedious to update all newly created .yml file with the property.

This PR adds an optional dbt_invoke_template.yml that will add default properties to the files updated by dbt-invoke.

What changes:

  1. load the contents of dbt_invoke_template.yml in _util.py,
  2. update the _structure_property_file_dict to apply the template's default properties.

I've also updated the README.md file so you can better understand how this works, and I've tested this by updating my local dbt-invoke package.

(feel free to edit anything on this; I'd love to have this functionality, so I'm saving the frustration of manually adding meta.owner)

Copy link
Member

@robastel robastel left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

And thanks also for the context you already provided. I added a few initial comments/questions so that I can further develop my understanding of how this feature may be used.

dbt_invoke/internal/_utils.py Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
if the key isn't present, it will break what's 
in dbt_invoke/properties.py

Co-authored-by: Robert Astel <[email protected]>


```
model:
Copy link
Member

Choose a reason for hiding this comment

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

I might be nitpicking a bit here, but I think we should change model, seed, snapshot, and analysis to their plural versions; models, seeds, snapshots, and analyses so that dbt_invoke_template.yml matches the convention used in property files. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you! I think it's better to align to the used convention, I'll update properties.py to use plural versions instead.

:return: None
"""
for key in template:
if key not in property_dict:
Copy link
Member

Choose a reason for hiding this comment

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

What if the key is already in the property_dict, but we had altered dbt_invoke_template.yml between the previous run and the current run?

Copy link
Author

@david-whimsical david-whimsical Nov 2, 2023

Choose a reason for hiding this comment

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

We would keep the value already in the file – else I think it could lead to frustrating behaviors, where dbt-invoke overwrites things.

In my opinion, The template should only add new lines, and not update any existing ones, if that makes sense!

dbt_invoke/properties.py Outdated Show resolved Hide resolved
@robastel
Copy link
Member

Given that this feature is totally new to dbt-invoke, we are going to need to add some tests, most likely to the tests/test_properties.py file.

A few test scenarios that immediately come to mind and that we should ensure work (both for the custom default resource properties and custom default column properties) are:

  • That everything still works as is when there is no dbt_invoke_template.yml file
  • That the output matches the expected output when there is a dbt_invoke_template.yml
  • If the value of a key in dbt_invoke_template.yml is updated, and that key already exists in the property file, then the value of that key should be updated in property file next time dbt-invoke properties.update is run

Were there any interesting scenarios or issues you noticed during your testing or any other scenarios you can think of that should be tested?

Something else I am pondering: What happens if the dbt_invoke_template.yml file contains invalid yml, is empty, or contains none of the supported resource types? In your opinion, what should happen in these cases?

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.

2 participants