Dont append to slices in mergeStruct

[sparrc]

@gotyaoi, I believe this is a change that you made for supporting directories. Can you explain why slices get appended but all other data structures get replaced here? If there isn't a strong reason to keep it I'd rather it just have a consistent behavior for all data types

I'm having an issue now where we're getting all slices doubled from the config to structs

[gotyaoi]

Since slices could hold more than one value, my thought was that specifying all values if a particular part of the config were re-specified at a lower level would be undesirable. I would have done the same for maps, but they don't appear to be produced by our toml library, according to it's docs.

For example, with the disk plugin, start with a restrictive list of mountpoints, then expand it in the config directory.

Regarding the doubling, that appears to have been introduced here: b10b186#diff-2fe34e1b6f7f9aece0af74b1635502eeL181

It takes the plugin struct from the config, then passes it to ApplyPlugin, which is causing the doubling. The previous way, where the agent first created an empty plugin and then passed that to ApplyPlugin was to allow for some defaults to be specified by the agent, but overriden if necessary. If that's not necessary, then the plugin value retrieved by PluginsDeclared should already be enough, though that would break down if we needed to do things in ApplyPlugin in the future.

[sparrc]

@gotyaoi I'm not sure I agree with having the ability to append to lists at lower levels of the config, that seems unintuitive to me. Can you explain what exactly the use-case there is?

[gotyaoi]

Well, to expand on the example of the disk plugin, in the main config, you would have this section:

[disk]
mountpoints = ["/"]

then in the conf directory, you would have:

[disk]
mountpoints = ["/data"]

Which would get merged into:

[disk]
mountpoints = ["/", "/data"]

When you're working with a config management system, it can be nice to have this, as you have some common config that gets distributed to all nodes in the the main config file, then individual nodes or classes of nodes get assigned customizations as necessary. It's not absolutely necessary though. I think I was mostly inspired by the configuration system in fail2ban, which can be fairly complicated, so perhaps I was overthinking it.

[sparrc]

OK, it makes sense, I'll have to think it over some more. Do you know if this is the only thing configuration-wise that relies on this behavior?

[gotyaoi]

I don't believe that anything besides the tests rely on the appending to arrays behavior.

[sparrc]

@gotyaoi I think I would prefer to just have all types overwrite in the config directories, to me this seems more intuitive, and I think it's fairly easy to workaround with a config management system. You just need to have your config management have

[disk]
mountpoints = ["/", "/data"]

rather than just "/data". I know that this introduces some duplication of effort, but I still think that being able to say that configurations within directories always overwrite is worth it.

The 2nd thing I did was remove the call to ApplyPlugin(). I'm not 100% sure I understand what was happening here but from what I could tell, we already applied the plugin configuration when we call LoadPlugin, and we can just get the plugin config directly. If you know of something I might be missing there please let me know!

[gotyaoi]

Fair enough.

The reasoning behind the ApplyPlugin, I believe, is to apply the plugin struct held by the config onto the plugin struct held by the agent. This would allow the agent to perform some default configuration of the struct, but allow that default configuration to be overruled by the config. You can see it more clearly in the agent's call to ApplyAgent: https://github.com/influxdb/telegraf/blob/master/agent.go#L73. The NewAgent function creates a default Agent struct, then calls ApplyAgent to fill in the details from the user supplied config. That's why prior to b10b186 LoadOutputs and LoadPlugins created a default output and a default plugin respectively and then passed those to ApplyPlugin/ApplyOutput. No default configuration was being done though, I believe.