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/configschema] use contrib components, simplify grepmod code #12620

Merged
merged 9 commits into from
Aug 16, 2022
Merged

[cmd/configschema] use contrib components, simplify grepmod code #12620

merged 9 commits into from
Aug 16, 2022

Conversation

pmcollins
Copy link
Member

@pmcollins pmcollins commented Jul 20, 2022

The configschema module makes available an API to get metadata about component configs. It also provides a command, docsgen, to generate markdown files for each component, describing its configuration fields, types, and default values.

This functionality got moved from core a few months back, but there was some additional work remaining. This change finishes that work, making configschema compatible with contrib.

This change also adds a makefile target, which runs docsgen all, writing over 140 markdown files to their corresponding component directories. For reference, one example markdown file is included in this PR (for the Redis receiver).

We will likely want to get a markdown generation job running as well, but that will be handled as a separate effort.

Addresses #12639

@pmcollins pmcollins marked this pull request as ready for review July 21, 2022 20:12
@pmcollins pmcollins requested review from a team and dmitryax as code owners July 21, 2022 20:12
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

I am a complete newbie with configschema, but I am interested on this for improving documentation, so I left some comments on the generated file. Most (all?) of these don't need to be dealt with on this PR, but if we agree that they should be improved, we can create issues for them

receiver/redisreceiver/config.md Outdated Show resolved Hide resolved
receiver/redisreceiver/config.md Outdated Show resolved Hide resolved
receiver/redisreceiver/config.md Outdated Show resolved Hide resolved
receiver/redisreceiver/config.md Show resolved Hide resolved
receiver/redisreceiver/config.md Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@dmitryax
Copy link
Member

Please take a look at the failing CI checks

@dmitryax
Copy link
Member

@pmcollins can you please fix the failing CI checks and rebase?

@dmitryax
Copy link
Member

Failing tests seem to be related. In order to make markdown-link-check tool happy, you'd probably need to add another section in https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/.github/workflows/check_links_config.json#L2 to ignore anchor links

@pmcollins
Copy link
Member Author

Hi @dmitryax -- all checks have passed 🎉

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing it!

I'm still not sure that the format that we get as the result is what we would like to keep going forward.

Do you have any other ideas on how to improve readability?

I'm curious if regular yaml would be an option to consider, e.g.:

// Doc string for the `option_1` taken from the code.
option_1: "default_value"

// Doc string for `option_2`.
option_2: 

  // Doc string for `embedded_field_1`.
  embedded_field_1: 0

  // Doc string for `embedded_field_2`.
  embedded_field_2: ""

WDYT?

I think we should probably create an issue to discuss how we want to evolve this functionality.

@dmitryax
Copy link
Member

dmitryax commented Aug 16, 2022

Oh I see you created #13384 already. Let's move the discussion there

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

4 participants