8000 adding the ability to get the root configuration by shawkins · Pull Request #39123 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

adding the ability to get the root configuration - #39123

Closed
shawkins wants to merge 1 commit into
keycloak:mainfrom
shawkins:rootConfig
Closed

adding the ability to get the root configuration#39123
shawkins wants to merge 1 commit into
keycloak:mainfrom
shawkins:rootConfig

Conversation

@shawkins
@shawkins shawkins commented Apr 22, 2025
Copy link
Copy Markdown
Contributor

No description provided.

@shawkins
adding the ability to get the root configuration
f8f4355 
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins
shawkins commented Apr 22, 2025
Copy link
Copy Markdown
Contributor Author

@keycloak/cloud-native @pruivo does this look like something you would be ok moving forward with? I'm just reusing most of the Scope class via BaseScope because we can't directly use Options here as we're all the way down in core.

If this seems good, I'll open an issue.

@pruivo
pruivo commented Apr 22, 2025
Copy link
Copy Markdown
Member

@shawkins it looks good to me. I don't think you need to split the interface in two; you could add root() to the Scope.
Does it make sense if scope() (invoked with no arguments) to return the "root" (instead of the new method)?

@shawkins
shawkins commented Apr 22, 2025
Copy link
Copy Markdown
Contributor Author

@shawkins it looks good to me. I don't think you need to split the interface in two; you could add root() to the Scope.

My reasoning is:

  • you don't need root() if you are already at root()
  • the propertyNames methods should be deprecated, so we don't want that exposed anymore
  • we don't really think of the root configuration as having scopes, so there isn't a need to have the scope method there.

Does it make sense if scope() (invoked with no arguments) to return the "root" (instead of the new method)?

The scoping logic provides child scopes. If you call scope() currently it is a no-op.

@pruivo
pruivo commented Apr 23, 2025
Copy link
Copy Markdown
Member

we don't really think of the root configuration as having scopes, so there isn't a need to have the scope method there.

It may be useful to access other scopes but I'm not sure if it a widespread use case. I'm doing the following in my code:

        //legacy option, for backwards compatibility --spi-connections-infinispan-quarkus-site-name
        var legacySiteName = Config.scope(InfinispanConnectionSpi.SPI_NAME, "quarkus").get("site-name");
        if (legacySiteName != null) {
            logger.warn("--spi-connections-infinispan-quarkus-site-name is deprecated and may be removed in the future. Use --spi-cache-embedded-%s-site-name".formatted(DefaultCacheEmbeddedConfigProviderFactory.PROVIDER_ID));
        }
        var siteName = config.get(DefaultCacheEmbeddedConfigProviderFactory.SITE_NAME, legacySiteName);

Anyway, the PR looks good to me 👍

@vmuzikar
vmuzikar self-requested a review April 23, 2025 09:19
@shawkins
shawkins commented Apr 23, 2025
Copy link
Copy Markdown
Contributor Author

we don't really think of the root configuration as having scopes, so there isn't a need to have the scope method there.

It may be useful to access other scopes but I'm not sure if it a widespread use case. I'm doing the following in my code:

        //legacy option, for backwards compatibility --spi-connections-infinispan-quarkus-site-name
        var legacySiteName = Config.scope(InfinispanConnectionSpi.SPI_NAME, "quarkus").get("site-name");
        if (legacySiteName != null) {
            logger.warn("--spi-connections-infinispan-quarkus-site-name is deprecated and may be removed in the future. Use --spi-cache-embedded-%s-site-name".formatted(DefaultCacheEmbeddedConfigProviderFactory.PROVIDER_ID));
        }
        var siteName = config.get(DefaultCacheEmbeddedConfigProviderFactory.SITE_NAME, legacySiteName);

Anyway, the PR looks good to me 👍

Another motivation behind this change is that we pass a Scope to the init method, so it's an "official" stateful way of obtaining configuration information. Accessing static methods on Config, and elsewhere, is something we should eventually refactor out of the public api - in particular there's no reason someone should call Config.init. cc @pedroigor

The point is taken that you using the scope method to access another provider's configuration, rather than using a constant or convienence method based around toDashCase. So I'll open an issue and refine this pr to being just a single interface instead.

@shawkins shawkins mentioned this pull request Apr 24, 2025
Merged
@pruivo pruivo mentioned this pull request Apr 28, 2025
Merged
4 tasks done
@shawkins shawkins mentioned this pull request May 21, 2025
Closed
2 tasks done
@mabartos
mabartos commented Jun 16, 2025
Copy link
Copy Markdown
Member

@shawkins What is the status of this PR?

@shawkins
shawkins commented Jun 16, 2025
Copy link
Copy Markdown
Contributor Author

@shawkins What is the status of this PR?

I incorporated much of the modifications into another pr, so I just need to revisit this and clean it up.

@shawkins
shawkins commented Jun 20, 2025
Copy link
Copy Markdown
B564 Contributor Author

Looking at this again it's probably not worth messing with until we make a more comprehensive attempt at not exposing all of static Config methods.

@shawkins shawkins closed this Jun 20, 2025
@shawkins shawkins mentioned this pull request Jul 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar Awaiting requested review from vmuzikar
Copilot code review Copilot
Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shawkins @pruivo @mabartos
0