-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add a filter to only include *.conf files from collectd.d. #120
base: master
Are you sure you want to change the base?
Conversation
Include "/opt/stackdriver/collectd/etc/collectd.d" | ||
Include "/etc/stackdriver/collectd.d" | ||
# If you have other config, especially for plugins, you can drop them |
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.
s/config/configs
@@ -124,7 +124,10 @@ PostCacheChain "PostCache" | |||
</Rule> | |||
</Chain> | |||
|
|||
# if you have other config, especially for plugins, you can drop them | |||
# into this directory | |||
# Obsolete config directory for backward compatibility. |
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.
Users could have put files into the other dir without the .conf
extension, right? Since that's already not totally backwards-compatible (changing that one to take .conf
only), I'd say for consistency we should either:
- get rid of 2 dir options and just leave the new block including
.conf
(131-133), or - have both dir options, and they both only accept
.conf
files
(i'm partial to the former)
@@ -1,3 +1,3 @@ | |||
# The release version and build number of the stackdriver-agent package. | |||
PKG_VERSION=6.1.3 | |||
PKG_VERSION=6.2.0 |
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.
Hmm, I have very contradicting feelings about this. I don't consider this to be a change in the API because the agent still behaves exactly the same for its purpose of collecting/sending metrics. The only change is to a config file that isn't fixed -- and that configurability is a "part of the API" (eh, sort of, maybe, not really, but you get it). That part of me wants to say that this doesn't even deserve a patch bump, but I do think there's merit in being able to differentiate between versions of the agent with the default changed vs not -- thus I'd vote patch bump.
(side question: if a user upgraded, their config wouldn't be overwritten with this right? (and yes yes, templates and new VMs that don't have one to begin with, inconsistencies etc etc))
If we /do/ consider a config file (that's shipped with the agent by default but configurable by the user by feature) to be a core part of the API, then I'd say this should be a major version bump.
No description provided.