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

4.x: standardize use of GlobalConfig by Helidon components #7627

Closed
barchetta opened this issue Sep 19, 2023 · 5 comments
Closed

4.x: standardize use of GlobalConfig by Helidon components #7627

barchetta opened this issue Sep 19, 2023 · 5 comments
Assignees
Labels
4.x Version 4.x builder Related to the builder support config
Milestone

Comments

@barchetta
Copy link
Member

barchetta commented Sep 19, 2023

Currently we have some inconsistencies with the use of GlobalConfig by Helidon components. For example WebServer requires the following:

        Config config = GlobalConfig.config();
        WebServer server = WebServer.builder()
                .config(config.get("server"))

But other components, like ObserveFeature, do not require the passing of config. For example the constructor for ObserveFeature.Builder uses GlobalConfig by default:

private Builder() {
     config(GlobalConfig.config().get("observe"));
}

In general I think discovering config by default is a good thing, and I'm not sure why we are not doing it for WebServer. Some things to consider

  1. Make sure all Helidon components that have config use GlobalConfig by default (including WebServer)
  2. Discuss if we should provide a way to force Builders to NOT use GlobalConfig, so that one may truly build from scratch with no interference.
  3. Discuss if we should require the user to always explicitly set GlobalConfig and not implicitly initialize it. Builders could either fail or conditionally process global config if set.

Once we settle on the preferred pattern for initializing config we need to ensure all archetypes and examples show that (see issue #7628).

@tomas-langer
Copy link
Member

The GlobalConfig is intended for components that are initialized automagically
(such as MetricsRegistry). There are some leftovers from the prototype where we did not support config on APIs. I am not sure we want to pick the config automatically, as our APIs should have the config method and honor it.

@romain-grecourt
Copy link
Contributor

ObserveFeature.builder() currently calls ObserveFeature.Builder.config(Config) with GlobalConfig.config().
I just searched for the usage of GlobalConfig in metrics but did not find it.

Can we consider reverting all current SE usages of GlobalConfig for consistency ?

@tomas-langer
Copy link
Member

GlobalConfig should be used for componets that require static setup. It should not be used for components that are configured by hand.

@barchetta
Copy link
Member Author

barchetta commented Sep 21, 2023

See PR #7625 and #7655
To Summarize:

  • Config must be explicitly passed to components (as is shown above for WebServer)
  • The Gobal config can be retrieved using Config.global() (and set with Config.global(config))
  • Not setting the global config uses the default config and is therefore the same as Config.global(Config.create())

@barchetta
Copy link
Member Author

With the above two PRs this issue has been addressed.

@m0mus m0mus added this to Backlog Aug 12, 2024
@m0mus m0mus moved this to Closed in Backlog Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x builder Related to the builder support config
Projects
Archived in project
Development

No branches or pull requests

3 participants