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

[config] Fix _is_affirmative when passed argument is None #3063

Merged
merged 1 commit into from
Dec 6, 2016

Conversation

olivielpeau
Copy link
Member

What does this PR do?

Avoid raising exception when passed argument to _is_affirmative is None.

Return False in that case instead of raising an exception.

For instance #3010 introduced a case where the passed argument is None, which would make the whole collector fail. #3010 should not be released without this fix

Motivation

Fix stuff

Testing Guidelines

It works.

Returning False doesn't break backwards compatibility because that made the code blow up before.

Additional Notes

🤖

Return `False` in that case instead of raising an exception.
Copy link
Member

@degemer degemer left a comment

Choose a reason for hiding this comment

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

👍
You can ignore my comment

@@ -222,6 +222,8 @@ def _checksd_path(directory):


def _is_affirmative(s):
if s is None:
Copy link
Member

Choose a reason for hiding this comment

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

return s is None or isinstance(s, int) and bool(s) or s.lower() in ('yes', 'true', '1')

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree but for some reason I find things more readable the way they are, so I'll ignore this

Copy link

Choose a reason for hiding this comment

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

especially that @degemer 's version is wrong. It would return True if s is None ..

@olivielpeau olivielpeau added this to the 5.11.0 milestone Dec 6, 2016
@olivielpeau olivielpeau merged commit de297cc into master Dec 6, 2016
@olivielpeau olivielpeau deleted the olivielpeau/is-affirmative-none-fix branch December 6, 2016 23:08
degemer added a commit that referenced this pull request Dec 21, 2016
* master: (53 commits)
  [nginx] Update example config
  [service_discovery] Add a Zookeeper service discovery implementation.
  [aggregator] if sample rate is bad, fix it but still parse tags. (#3073)
  [yarn] whitelist authorized application_tags
  Alex poe/update jmx with refresh beans (#3068)
  [config] Fix `_is_affirmative` when passed argument is `None` (#3063)
  Send all configured tags with process checks. (#2976)
  fix flake8 errors
  [flare] ignore whitespace before proxy credentials
  [core] add a switch to disable profiling, but still use developer mode (#2898)
  [tests] allow tests to use the additional_checksd parameter (#3056)
  [service_discovery][jmx] trying to pick-up JMX changes with SD. (#3010)
  [install_script] Make `dd-agent` group of `datadog.conf` (#3036)
  [postgres] Allow disable postgresql.database_size (#3035)
  [core] Fixes IndexError for process lookup (#3043)
  remove warning message leaking password strings (#3053)
  trap psutil.NoSuchProcess exception (#3052)
  Fix grammar and casing in exception text (#3050)
  allow override of kubelet host with KUBERNETES_KUBELET_HOST env var
  [service discovery] properly handle config reload for removed containers
  ...
@masci masci modified the milestones: 5.11.0, 5.12.0 Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants