-
Notifications
You must be signed in to change notification settings - Fork 253
Removing exclude_resource_type functionality since it's broken #2005
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.
|
|
@anyapriya, are you considering these changes as a bug fix? Could you please share an example to help me better understand the issue you're trying to address? |
@pankajastro As mentioned in the linked issue, if you use the following settings: Then it results in this error: I think it should be removed for the following reasons:
As mentioned in the issue, I'm open to alternative options after input from the cosmos team, but went this route after not hearing any response, and because I ultimately think it's the cleanest approach for the reasons stated above. I think if there really is an interest in adding exclude resource type functionality, then it should be added as its own flag as a separate parameter instead of as part of the select / exclude parameters, along with the resource type flag for consistency, but that seems like a decent amount of work to implement with little value beyond what already exists. |
|
I'm seeing that 5 tests are failing. It seems like the failing tests are due to the following:
Neither seems related to this PR, would love recommendation from the cosmos team on how to handle this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the broken exclude_resource_type functionality that was incorrectly implemented as a tag-based selector instead of a proper flag. The functionality was not working as intended because it mixed up dbt's --exclude-resource-type flag with selector syntax.
- Removes all
exclude_resource_typeselector code and related functionality - Updates documentation to remove references to the broken feature
- Removes associated tests that were testing the broken implementation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cosmos/dbt/selector.py | Removes exclude_resource_type selector constants, parsing logic, and filtering methods |
| docs/configuration/selecting-excluding.rst | Updates documentation to remove exclude_resource_type selector reference |
| tests/dbt/test_selector.py | Removes test cases for the broken exclude_resource_type functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2005 +/- ##
==========================================
+ Coverage 97.06% 97.77% +0.71%
==========================================
Files 91 91
Lines 5862 5850 -12
==========================================
+ Hits 5690 5720 +30
+ Misses 172 130 -42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@anyapriya I agree with your analysis. It was a mistake to accept that contribution. I'm talking to the original contributor of this feature next week to confirm the possibility of them changing how they accomplish this, before we go ahead to remove the feature. |
Description
Essentially undoes this PR.
While dbt did the ability to use --exclude-resource-type flag (which matches --resource-type that already exists), it is meant as a separate flag. The way this was implemented was mixing it up with the "resource_type:" tag that can be used as part of the "--select" flag - there is no corresponding "exclude_resource_type:" tag in dbt. As a result, it wasn't working the way it was intended as explained in this issue.
It's also unclear what putting a tag of exclude_resource_type into the "exclude" parameter is meant to do.
In order to add --exclude-resource-type correctly, it would need to be properly implemented as a flag instead of a tag which I think is a much bigger change.
Additionally, I can't think of any situations where this would be needed instead of using the "resource_type:" tag in the exclude parameter of the render config.
Related Issue(s)
Closes #1733
Breaking Change?
Sort of : removes functionality, but the functionality was broken.
Checklist