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

ConfigSelectorMethod should check for bools #5889

Merged
merged 5 commits into from Sep 21, 2022

Conversation

danielcmessias
Copy link
Contributor

@danielcmessias danielcmessias commented Sep 20, 2022

resolves

resolves #5890
resolves #5482

Description

The ConfigSelectorMethod only checks for string equality. If you have a config property that is boolean, there is no way to match it.

Consider the example:

# schema.yml
models:
  - name: some_model
    config:
      meta:
        pii: true

Then running dbt run --select config.meta.pii:true would not match. The only way for it work would be to encapsulate the property as a string (pii: "true"), not ideal.

This is a small change such that if the property is a bool, check (case-insensitively) against the strings "true" / "false".

Checklist

@danielcmessias danielcmessias requested review from a team and stu-k September 20, 2022 17:13
@cla-bot
Copy link

cla-bot bot commented Sep 20, 2022

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @danielcmessias

@gshank
Copy link
Contributor

gshank commented Sep 20, 2022

Could you add a test? test/unit/test_graph_selector_parsing.py looks like a good place...

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Thanks @danielcmessias! Do you feel up for adding some test cases? Relevant unit tests are here.

I'm going to mark this as ready_for_review in the meantime

Edit: Gerda beat me to it :)

@@ -356,7 +356,10 @@ def search(
except AttributeError:
continue
else:
if selector == value:
# if the config is a bool then check against the strings "true"/"false"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we support all forms of yaml truthy/falsy (0/1, "t/"f", yes/no)?

I'm fine with just "true" + "false" personally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in favour of just true/false and avoid the yaml-specify truthy/falsy rules which don't apply in Python/JSON/Jinja templates

# if the config is a bool then check against the strings "true"/"false"
if ((selector == value) or
(value is True and CaseInsensitive(selector) == "true") or
(value is False and CaseInsensitive(selector) == "false")):
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not too far out of scope — what do you think about pulling in #5482 / code like this while you're here?

@jtcohen6 jtcohen6 added Team:Execution ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Sep 20, 2022
@danielcmessias
Copy link
Contributor Author

@gshank @jtcohen6 Added a resolution for #5482 and some test cases.

Went back and forth a couple of times on the structure; idk whether separate clauses for the list and string case that both yield the node is more or less readable than one giant IF with all the conditions 🤷‍♂️ . LMK if you'd prefer it another way :)

@gshank
Copy link
Contributor

gshank commented Sep 20, 2022

Need to run pre-commit now, pip install pre-commit if it's not already installed, then run 'pre-commit run --all-files" and commit the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
3 participants