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

Work around config doc issues for LGTM #41291

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Jun 18, 2024

There are several issues:

  • first, the config doc processor is part of the extension processor for now, so you need to run it to analyze the config.
  • second we need to mark at least one class with @ConfigMapping, otherwise the config groups cannot be analyzed as part of config mappings. This will go away in the future.
  • then, the hierarchy here is quite weird with some methods overriding others, which is fine, except this is not properly handled in the config doc generator. For now, I just ignored the ones from the parent, but ideally, we should override them properly in the config doc generator.
  • also I removed some inherited methods from GrafanaConfig and I'm not yet sure if it will break something so creating as draft.

Fixes #41285

@gsmet gsmet requested a review from alesj June 18, 2024 15:24
Copy link

quarkus-bot bot commented Jun 18, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 226f281.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@alesj
Copy link
Contributor

alesj commented Jun 19, 2024

The hierarchy stuff, with dups, was to handle previous behavior, which didn't know about hierarchy.
But afair, this was fixed by @radcortez , right?

@alesj
Copy link
Contributor

alesj commented Jun 19, 2024

Otherwise, this LGTM (pun intended ;-).

@radcortez
Copy link
Member

The hierarchy stuff, with dups, was to handle previous behavior, which didn't know about hierarchy.
But afair, this was fixed by @radcortez , right?

I have a vague idea that we have some issue with that, but I can't remember exactly. Regardless, I believe any hierarchy mapping should work just fine.

There are several issues:
- first, the config doc processor is part of the extension processor for
  now, so you need to run it to analyze the config.
- second we need to mark at least one class with @ConfigMapping,
  otherwise the config groups cannot be analyzed as part of config
  mappings. This will go away in the future.
- then, the hierarchy here is quite weird with some methods overriding
  others, which is fine, except this is not properly handled in the
  config doc generator. For now, I just ignored the ones from the
  parent, but ideally, we should override them properly in the config
  doc generator.
- also I removed some inherited methods from GrafanaConfig and I'm not
  yet sure if it will break something so creating as draft.
@gsmet gsmet force-pushed the lgtm-config-doc branch from 226f281 to f0ecf28 Compare June 20, 2024 11:06
@gsmet gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes triage/backport labels Jun 20, 2024
@gsmet gsmet merged commit 36d6864 into quarkusio:main Jun 25, 2024
18 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 25, 2024
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jun 25, 2024
@gsmet gsmet modified the milestones: 3.13 - main, 3.12.1 Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config Doc - LGTM doc is not expanded
3 participants