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

Support env overrides for all inputs of the same type #6334

Merged
merged 1 commit into from
Apr 15, 2016

Conversation

gunnaraasen
Copy link
Member

@gunnaraasen gunnaraasen commented Apr 13, 2016

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

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:

[[graphite]]
  enabled = true
  bind-address = ":2003"

[[graphite]]
  enabled = true
  bind-address = ":8085"

And setting the following environment variables:

# Override all Graphite input's bind address setting to :1003 
INFLUXDB_GRAPHITE_BIND_ADDRESS=:1003
# Override the second Graphite input's bind address setting to :3003
INFLUXDB_GRAPHITE_1_BIND_ADDRESS=:3003

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.

@gunnaraasen gunnaraasen force-pushed the ga-env-multiple-inputs branch 2 times, most recently from 09e10c4 to 41d4522 Compare April 13, 2016 03:58
@jsternberg
Copy link
Contributor

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 0 should refer to the first index in the slice, not the second.

@@ -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 {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

@gunnaraasen
Copy link
Member Author

gunnaraasen commented Apr 15, 2016

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.

@gunnaraasen gunnaraasen added this to the 0.13.0 milestone Apr 15, 2016
@e-dard
Copy link
Contributor

e-dard commented Apr 15, 2016

LGTM 👍

@jsternberg
Copy link
Contributor

👍

@gunnaraasen gunnaraasen merged commit 8682dc8 into master Apr 15, 2016
@gunnaraasen gunnaraasen deleted the ga-env-multiple-inputs branch April 15, 2016 16:51
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.

None yet

3 participants