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

[cmd/mdatagen] add a way to generate configuration as part of README #23054

Closed
wants to merge 6 commits into from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jun 4, 2023

Description:
Add a way to generate configuration as part of README.

Link to tracking Issue:
Not yet.

Testing:
Unit tests

Documentation:
N/A

@github-actions github-actions bot requested a review from dmitryax June 4, 2023 08:24
@atoulme atoulme added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 4, 2023
@atoulme atoulme force-pushed the generate_config branch 3 times, most recently from 436dec0 to 3c88bc4 Compare June 5, 2023 15:47
@dmitryax
Copy link
Member

dmitryax commented Jun 5, 2023

This is amazing!

Just one note that we have https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/cmd/configschema, which does a similar thing. The problem with the configschema is that it takes all the components as dependencies instead of parsing the config.go files as you are doing here. I believe the parsing is the best option, but I'm not sure it should be in mdatagen. I think mdatagen should generate stuff from metadata.yaml only. It'd be great if we could change the cmd/configschema to use this approach instead unless we want to define the config in the metadata.yaml in the first place and generate both docs and config.go -- which can be a pretty good option, just need to see how feasible is that.

I also suggested this format some time ago, but I like yours as well, it's just maybe harder to copy-paste for the end users. PTAL, what do you think.

@atoulme
Copy link
Contributor Author

atoulme commented Jun 5, 2023

Thank you, I'll take a look. This is just a skunkworks weekend thing and it needs to be tendered to in the right way. All your suggestions help.

@dmitryax
Copy link
Member

dmitryax commented Jun 7, 2023

@atoulme #23061 is a good example of how I envisioned the configuration definition in yaml format

@atoulme
Copy link
Contributor Author

atoulme commented Jun 7, 2023

Definitely seems like a good format to me. Especially because you'd then copy/paste this into your collector config, right?

I haven't had time to get back to this code yet.

@dmitryax
Copy link
Member

dmitryax commented Jun 7, 2023

Especially because you'd then copy/paste this into your collector config, right?

Exactly

@atoulme atoulme force-pushed the generate_config branch 4 times, most recently from c1e507f to b946520 Compare June 9, 2023 02:18
@dmitryax
Copy link
Member

dmitryax commented Jun 9, 2023

I don't like that we introduce another package doing pretty similar to what configschema is doing. Maintaining them both is not ideal. I'd be happy to replace it, but we need a plan.

One suggestion:

  1. Refactor configschema to use your approach with generate pragmas -- this will help the CI time a lot!
  2. Make configschema to produce the yaml format embedded into the README instead of https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/redisreceiver/config.md

Another suggestion:

  1. Define the configuration schema as yaml (probably inside metadata.yaml under config), similar to what configschema used to generate. Use it as a source of truth and generate both the config.go and README.md files.

I like the second one, but it might be a lot of work.

@atoulme
Copy link
Contributor Author

atoulme commented Jun 9, 2023

I get it. It’s a spike and I learned something from it: we will need to do more to get default values, reading the ast tree is not enough.

Sounds to me like configschema can generate all the yaml files, and we use those to generate config and README then. Eventually configschema can be deprecated and removed once we stabilize the yaml files approach.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 24, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/mdatagen mdatagen command exporter/splunkhec Skip Changelog PRs that do not require a CHANGELOG.md entry Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants