Repository navigation
Conversation
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class Property { |
There was a problem hiding this comment.
I'm wondering if this type should be added to the configuration API itself. We are using a similar structure when generating docs. See
.There was a problem hiding this comment.
I was wondering if I should try to re-use the docs generation, but I have no problems in including Property/Options as an extra class in the org.keycloak.quarkus.runtime.configuration package.
There was a problem hiding this comment.
I think that would be awesome but I don't think it is a blocker.
| .to("quarkus.http.port") | ||
| .defaultValue(String.valueOf(8080)) | ||
| .type(Integer.class) | ||
| .converter(Integer::parseInt) |
There was a problem hiding this comment.
Perhaps infer the converter based on the type and make the config even simpler?
There was a problem hiding this comment.
I like to keep the converter separated here since it's explicitly handled by the Picocli implementation and it can be used for multiple different purposes.
There was a problem hiding this comment.
I'm not saying you should remove the converter method from the API but infer the converter based on the type. The code above would just use:
type(Integer.class)
There was a problem hiding this comment.
What you are proposing is partially overlapping with picocli functionalities.
E.g. specifying an explicit type will make the default converter redundant IIUC
There was a problem hiding this comment.
I think we are talking about the same thing :) Let me ask then: Why do we need to explicitly set the converter?
There was a problem hiding this comment.
it was a nice extension point to build upon validation etc., I can probably take it off from the follow-up PRs to reduce the scope of the task
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd"> | ||
| <parent> | ||
| <artifactId>keycloak-docs-parent</artifactId> |
There was a problem hiding this comment.
I know it's just a demo but wanted to point out that we're inheriting from docs module for some reason. Is that intended?
There was a problem hiding this comment.
not intended, will be removed if/when we bring this forward
| if (configurations.containsKey(k)) { | ||
| Set<Property> tmpValue = configurations.get(k); | ||
| tmpValue.add(v); | ||
| configurations.put(k, tmpValue); |
There was a problem hiding this comment.
Just a nitpick (for when this is not a demo anymore :)). I believe you don’t need to put the Set back again into the Map. It's already a reference.
| import static io.smallrye.config.common.utils.StringUtil.replaceNonAlphanumericByUnderscores; | ||
|
|
||
| @Mojo(name = "keycloak-operator-codegen", defaultPhase = LifecyclePhase.GENERATE_SOURCES) | ||
| public class CodegenMojo extends AbstractMojo { |
There was a problem hiding this comment.
Was wondering if we should not have have it the other way around. Some POJO with few custom annotations on top of field as the source of truth. This would get translated into the mappers. Might be a bit cleaner than a maven plugin for basically generating classes on the fly?
In any case, might be worth exploring what Store.X team will be doing. As mentioned, I think it is related to this, so might make sense to align on it.
There was a problem hiding this comment.
Was wondering if we should not have have it the other way around. Some POJO with few custom annotations on top of field as the source of truth.
This would require a pretty invasive change on how the CLI and configuration work as of today, I would like to hear from @pedroigor and @DGuhr
But, also, let's first open a GH "discussion" about the desired outcome and tackle the technical implementation as a separate subject.
There was a problem hiding this comment.
My PoC actually had a POJO inside that could be used/extended as source of truth (the pojo is actually applicable to all sorts of "PortConfiguration", so the actual configurationkey POJO would extend it and add e.g. name, description, and so on), and I am pretty paranoid due to bad experiences when it comes to code generators, so +1 from my side. Nevertheless I totally agree this is more invasive, needs a bit of boilerplate work and should be discussed first.
For the problem space / the discussion part, a primer:
e.g. I thought the goal here was not only for the operator to get the config classes, but to extract a configuration package with its own api/interface, that could be used by any frontend, be it tools like https://github.com/adorsys/keycloak-config-cli or "I want to write my own keycloak configuration service using e.g. jbang", or "i want to set keycloak configuration using an existing tool like spring cloud config server" or even "I want to have an intellij / vscode plugin to show me when I have an invalid configuration even before starting up anything, so immediately when typing e.g. proxy=reencrypt, http-enabled=true". This, together with a --no-validate flag or sth around these lines that skips validation at startup bc. "hey, i know what i am doing and it was validated beforehand", would lead to the outcome that configuring keycloak would be much more flexible to do with whatever tool you want, would be decoupled from the actual runtime keycloak lives in, as the config is "self-contained" (given we also abstract away the "to" part from the mappers, this could even lead to a wildfly configuration frontend ;) ) bc validating could be done without even invoking keycloak, so at startup we would have nearly no impact on performance.
There was a problem hiding this comment.
@DGuhr +1 to that it'd also be beneficial in general – decoupling our configuration logic from Quarkus.
|
Superseded by: #11929 |
This is a demo showing a possible (initial)flow from Quarkus distribution server configuration to Operator CRD typed generation.
http-port)maven-pluginto generate the needed POJOs from the aggregatedPropertyMappersKeycloakSpecand got a CRD generatedA few considerations:
maven-pluginand the codegen is pretty much hacked together (don't look at code-style etc. it only serves demo purposes)maven-pluginto generate sources for the operator we do not have issues around classpath/Quarkus versions/etc. etc. nor we need a completely independent maven sub-project to refactor the configuration (even if it would be nice, but it's decoupled).cc. @DGuhr @vmuzikar @pedroigor
Exploratory work to target #10866