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

change boolean values in config from yes/no to true/false #2078

Merged
merged 3 commits into from
Aug 22, 2018

Conversation

arbll
Copy link
Member

@arbll arbll commented Aug 20, 2018

What does this PR do?

Changes all boolean values in agent 6 config files from yes/no to true/false.

Motivation

Sister of DataDog/datadog-agent#2171
Main goal was to avoid confusion when using environment variable instead of config files as yes/no is not supported in that case.

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

@arbll arbll added this to the 6.5.0 milestone Aug 20, 2018
@arbll arbll requested review from a team as code owners August 20, 2018 13:50
gzussa
gzussa previously approved these changes Aug 20, 2018
Copy link
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

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

LGTM to me. However, make sure it is safe to do so in the code (good use of is_affirmative)

rishabh
rishabh previously approved these changes Aug 20, 2018
l0k0ms
l0k0ms previously approved these changes Aug 21, 2018
Copy link
Contributor

@l0k0ms l0k0ms left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@masci
Copy link
Contributor

masci commented Aug 21, 2018

Not sure what the reviews mean: did anybody check we're making good use of is_affirmative as @gzussa pointed out? Why approving if we're not sure?

@arbll
Copy link
Member Author

arbll commented Aug 21, 2018

As yes/no and true/false are supposed to be evaluated at the YAML level, instance.get(boolean_key) should return a bool (only doubt I have is with A6 if there is some weirdness with the go/python bridge). Worrying about proper use of is_affirmative only make sense if values are passed as string.

@masci
Copy link
Contributor

masci commented Aug 21, 2018

Thanks @arbll !

@arbll arbll dismissed stale reviews from l0k0ms, rishabh, and gzussa via d2e8b63 August 21, 2018 15:36
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.

5 participants