Skip to content

Consolidate default values for settings#312

Merged
masci merged 4 commits intomasterfrom
massi/set-defaults
Jun 16, 2017
Merged

Consolidate default values for settings#312
masci merged 4 commits intomasterfrom
massi/set-defaults

Conversation

@masci
Copy link
Copy Markdown
Contributor

@masci masci commented Jun 16, 2017

What does this PR do?

Consolidates the use of SetDefault for any configuration setting.

Motivation

Part of the defaults were set within packages, part in the common config package. Let's put them all in the same place so it's less error prone.

Copy link
Copy Markdown
Contributor

@gmmeyer gmmeyer left a comment

Choose a reason for hiding this comment

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

two questions but lgtm otherwise

api_key:

# If you need a proxy to connect to the Internet, provide it here (default: disabled)
# proxy: http://user:password@host:port
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why did you remove this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My bad, blindly removed anything didn't have a SetDefault

# proxy: http://user:password@host:port

# If you run the agent behind haproxy, you might want to set this to yes
# skip_ssl_validation: no
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(agreed, these 2 options are supported, I don't think we should remove them from here)

Copy link
Copy Markdown
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

This is great! ⭐️

Just a nit, but feel free to merge

Comment thread pkg/collector/dist/datadog.yaml Outdated
# additional_checksd:

# The path where the IPC api listens
# cmd_sock:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: for the 3 options above, it'd be good to document the default value (it might depend on the OS, in which case we can write a general description of the default, for example for additional_checksd: By default, uses the checks.d folder located in the agent configuration folder)

# proxy: http://user:password@host:port

# If you run the agent behind haproxy, you might want to set this to yes
# skip_ssl_validation: no
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(agreed, these 2 options are supported, I don't think we should remove them from here)

Comment thread pkg/collector/dist/datadog.yaml Outdated
# hostname: mymachine.mydomain

# The path containing check configuration files
# By default, uses the conf.d folder located in the agent installation folder.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agent configuration folder instead no? (same for option below)

Copy link
Copy Markdown
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

:shipit:

@masci masci merged commit 65508fb into master Jun 16, 2017
@masci masci deleted the massi/set-defaults branch June 16, 2017 19:09
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