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

Feature: merge multiple "--config" and "--config-directory" flags #9007

Merged
merged 5 commits into from May 18, 2021

Conversation

gdunstone
Copy link
Contributor

This adds a feature mentioned in this issue #7501

The only change in functionality is that telegraf will now merge multiple "--config" and "--config-directory" flags into the config, rather than only using value of the last flag.

Required for all PRs:

  • Associated README.md updated.
  • Has appropriate unit tests.

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

🤝 ✅ CLA has been signed. Thank you!

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Mar 18, 2021
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

While the code looks good to me, you have to fix the CI errors for the windows build before this has a chance to be merged. The error is

# github.com/influxdata/telegraf/cmd/telegraf
cmd\telegraf\telegraf_windows.go:87:7: undefined: fConfig
cmd\telegraf\telegraf_windows.go:88:48: undefined: fConfig
cmd\telegraf\telegraf_windows.go:90:7: undefined: fConfigDirectory
cmd\telegraf\telegraf_windows.go:91:77: undefined: fConfigDirectory

@srebhan srebhan self-assigned this Mar 18, 2021
@srebhan
Copy link
Contributor

srebhan commented Mar 18, 2021

And make fmt plz! :-)

@gdunstone
Copy link
Contributor Author

Sorry about that, I figured it was quicker to just fix it rather than reply.

Thanks for reviewing this.

@srebhan
Copy link
Contributor

srebhan commented Mar 18, 2021

!retry-failed

1 similar comment
@srebhan
Copy link
Contributor

srebhan commented Mar 19, 2021

!retry-failed

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Mar 19, 2021
@Hipska
Copy link
Contributor

Hipska commented Apr 13, 2021

Since, config-directory already walks recursively, I don't think it is needed to have the possibility to define multiple on the command-line?

@srebhan
Copy link
Contributor

srebhan commented Apr 14, 2021

@Hipska well imagine a case where you have one central (directory) repository that defines the "used-everywhere" plugins and then a machine-local directory for holding the machine-individual configs. Those will likely not be nested. We currently approximate such a use case with symbolic links in one "machine-local" directory but it's... a workaround. :-)

@Hipska
Copy link
Contributor

Hipska commented Apr 14, 2021

Both of these directories can be put in a parent directory and that one will be parsed recursively, so I still see no actual use case. It seems better to KISS as much as possible.

@srebhan
Copy link
Contributor

srebhan commented Apr 14, 2021

@Hipska let me put an example. The central repository is located on a server and mounted e.g. to /myserver/configs/telegraf, while on some of your machines you additionally want to put a config e.g. for special hardware on this machine only. Let's assume those config files reside on /usr/local/configs/telegraf. Of course you could now merge those two directories on a filesystem level (e.g. merge by copying the files, or (as I do) symlink the two, etc) but how is that better than being able to say
telegraf ... --config-directory /myserver/configs/telegraf --config-directory /usr/local/configs/telegraf?

@gdunstone
Copy link
Contributor Author

@Hipska Going through a directory recursively doesn't allow you to fetch and merge multiple config files over http.

@Hipska
Copy link
Contributor

Hipska commented Apr 21, 2021

Of course you could now merge those two directories on a filesystem level (e.g. merge by copying the files, or (as I do) symlink the two, etc) but how is that better than being able to say
telegraf ... --config-directory /myserver/configs/telegraf --config-directory /usr/local/configs/telegraf?

@srebhan symlink would indeed be better. I would avoid adding 'complex' logic on the command line arguments.

Going through a directory recursively doesn't allow you to fetch and merge multiple config files over http.

@gdunstone Correct, that's why I have absolutely no objection towards multiple --config arguments. I'm only against multiple --config-directory flags.

@gdunstone
Copy link
Contributor Author

@Hipska I am ambivalent about the --config-directory flag as I don't use it. My main objective with this PR was to allow merging multiple http configs with the --config flag, the --config-directory changes were just because it was convenient and I didn't see why not at the time.

However I can see the use case @srebhan provides.

@srebhan
Copy link
Contributor

srebhan commented Apr 22, 2021

@Hipska I still don't see the complex logic point, but to cut a long story short: I can live with only one config directory option even though I'd prefer multiple ones. ;-)

@Hipska
Copy link
Contributor

Hipska commented Apr 22, 2021

And I prefer not 😂 Best would be someone from Influx to give their thoughts.

If you want to include multiple different directories, I think it would be better to make that an option in the toml config rather than on the command line.

@srebhan
Copy link
Contributor

srebhan commented Apr 22, 2021

The only difference is: Now only you is happy, with the change we can both be happy as nobody forces you to use the option multiple times.

@Hipska
Copy link
Contributor

Hipska commented Apr 22, 2021

You are right 😛 But maybe put it on the table for the weekly meeting to know their view on it?

@ssoroka
Copy link
Contributor

ssoroka commented Apr 29, 2021

I'm okay with multiple --config and multiple --config-directory flags. It adds flexibility but doesn't harm the users who are not using it. This is good to go once my comment is resolved.

@gdunstone gdunstone requested a review from ssoroka May 13, 2021 02:19
@sspaink sspaink merged commit 3a1a44d into influxdata:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants