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

docs: Add sample-configuration for TLS client options #12728

Closed
wants to merge 3 commits into from

Conversation

srebhan
Copy link
Member

@srebhan srebhan commented Feb 23, 2023

Add a sample-configuration for the TLS client-config.

@telegraf-tiger telegraf-tiger bot added the docs Issues related to Telegraf documentation and configuration descriptions label Feb 23, 2023
@srebhan srebhan marked this pull request as draft February 23, 2023 15:07
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

overall love having this so we keep things in sync! just one question about naming that was throwing me off while reviewing this.

@srebhan
Copy link
Member Author

srebhan commented Apr 26, 2023

After some more thoughts this approach has several drawbacks:

  1. With plugins ending with a map definition in TOML e.g. [inpus.modbus.workarounds] this approach will not be possible and will generate malformed (and/or confusing) TOML files.
  2. The sample.conf files do no longer match the output of the SampleConfig function call.
  3. Inserting into a config structure is very limited as you can only add stuff at the beginning or end.
  4. There is no way to comment/uncomment an embedded config in a good way.

Given all those limitation (especially 1 and 2) I'm closing this PR as we need to come up with something more flexible.

@Hipska
Copy link
Contributor

Hipska commented Apr 27, 2023

1 could be solved by not including it for those kind of plugins, but I agree to close and find a better solution.

Maybe let sample.conf be a golang template where you include the tls.conf?

@srebhan
Copy link
Member Author

srebhan commented May 2, 2023

Maybe let sample.conf be a golang template where you include the tls.conf?

@Hipska that would be one way. In any way we should generate a sample.conf that is the rendered result and check that in to the repo. So this calls for another "generator" running with make docs...

@Hipska
Copy link
Contributor

Hipska commented May 4, 2023

I don't see the need for that, why would that rendered sample.conf need to be checked in the repo? (It only needs to be rendered in the readme, but that is currently already the case.)

In other words, why is N°2 a problem?

@Hipska
Copy link
Contributor

Hipska commented May 4, 2023

I do like the tls client.conf sample, that should be added to the repo anyway.

@srebhan srebhan force-pushed the tls_docs_enhancement branch from 95da167 to 9c858ea Compare May 26, 2023 12:50
@srebhan srebhan changed the title docs: Include TLS config docs: Add sample-configuration for TLS client options May 26, 2023
@srebhan srebhan marked this pull request as ready for review May 26, 2023 12:54
@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 May 26, 2023
@srebhan srebhan assigned powersj and unassigned srebhan May 26, 2023
@powersj
Copy link
Contributor

powersj commented May 31, 2023

@srebhan close this PR in favor of #13348 ?

@srebhan
Copy link
Member Author

srebhan commented Jun 2, 2023

Fine with me.

@srebhan srebhan closed this Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues related to Telegraf documentation and configuration descriptions 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.

3 participants