-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Support env overrides for all inputs of the same type #6334
Conversation
09e10c4
to
41d4522
Compare
So I'm clear on what this does, what does the current version allow and what are you trying to change? I only see the ability to specify one http section in the config at the moment so I'm not sure how the example in the explanation would work. For the unit test, I see it trying to set the zeroth UDP bind address, but it definitely seems like |
@@ -175,6 +175,9 @@ func (c *Config) applyEnvOverrides(prefix string, spec reflect.Value) error { | |||
// e.g. GRAPHITE_0 | |||
if f.Kind() == reflect.Slice || f.Kind() == reflect.Array { | |||
for i := 0; i < f.Len(); i++ { | |||
if err := c.applyEnvOverrides(key, f.Index(i)); err != nil { |
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.
This code specifically seems to be wrong. This code is confusing so I'm not certain, but I think this would allow me to say INFLUXDB_0_BIND_ADDRESS
to set all of the bind addresses in every section where that applies. If I'm reading this right, the unit test you have should work with the original code.
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.
The unit test doesn't work with the current code because inputs which allow multiple listeners don't get matched to environment variables like INFLUXDB_UDP_BIND_ADDRESS
, only INFLUXDB_UDP_[i]_BIND_ADDRESS
. This change allows both to be used to override the bind address for a section and only applies to sections which allow multiple instances, which are all input sections (UDP, graphite, etc).
Not sure how this would match a INFLUXDB_0_BIND_ADDRESS
env var to all bind addresses. Maybe I'm missing something.
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.
Ah, I think I misunderstood the f.Index(i)
code. I thought that was an integer index that got returned.
I opened #6333 to add support for multiple HTTP listeners but the example was poorly written. I've updated it to use a Graphite input and added a before and after description. Hope that helps clear up the confusing parts. |
41d4522
to
26c75a8
Compare
LGTM 👍 |
👍 |
26c75a8
to
d983a8c
Compare
Required for all non-trivial PRs
Allows a default environment variable override to be specified for all config config sections that are arrays. This keeps backward compatibility for existing environment variable overrides on sections which now allow multiple services to be configured due to #6228 and #6333.
Given the following configuration for just the Graphite sections:
And setting the following environment variables:
The current code will result in the first Graphite input using bind address
:2003
and the second Graphite input using a bind address of:3003
.This PR results in the first Graphite input using bind address
:1003
and the second Graphite input using a bind address of:3003
.