Skip to content

feat: lf_inherited_tags config to exclude certain tags from CRUD #453

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

Merged
merged 9 commits into from
Oct 17, 2023

Conversation

dacreify
Copy link
Contributor

@dacreify dacreify commented Oct 10, 2023

Description

See: #452

Acceptance Testing

  1. Associate a tag with a database
  2. Configure tag in dbt_project.yml, for example: +lf_inherited_tags: [my-tag] within models:
  3. Add an lf_tags_config to a model to set some other tags
  4. Run the model and observe that e.g. my-tag is retained instead of being removed and that additional model-level tags are associated

Checklist

  • You followed contributing section
  • You kept your Pull Request small and focused on a single feature or bug fix.
  • You added unit testing when necessary
  • You added functional testing when necessary

@nicor88
Copy link
Contributor

nicor88 commented Oct 11, 2023

Could you please fix the unit tests?

@dacreify
Copy link
Contributor Author

@nicor88 Ok fixed and confirmed locally. Not sure why it's not re-running the failed checks.

@svdimchenko
Copy link
Contributor

Overall the code looks OK to me with some minor adjustments. However I've left the comment in issue regarding this feature, I believe we should discuss it before proceesd with this PR.

@svdimchenko
Copy link
Contributor

@dacreify could you please add models examples with corresponding configs to PR description, this will make testing process easier to reviewers

@dacreify
Copy link
Contributor Author

@svdimchenko Sure added some testing steps -- see what you think

@svdimchenko
Copy link
Contributor

@dacreify I've tested your changes and the feature works as expected. So let's fix the unit tests, also there are some minor comments and we are ready to go

@svdimchenko svdimchenko added this to the v1.6.4 milestone Oct 13, 2023
@svdimchenko
Copy link
Contributor

@nicor88 @Jrmyy would you like to take a look at this PR as well ?

Copy link
Contributor

@nicor88 nicor88 left a comment

Choose a reason for hiding this comment

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

@dacreify great job. LGTM @svdimchenko

@svdimchenko svdimchenko merged commit 502939b into dbt-labs:main Oct 17, 2023
@dacreify
Copy link
Contributor Author

Thanks all!

@dacreify dacreify deleted the inherited-lf-tag-support branch October 17, 2023 15:42
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