Handle ignored artifacts separately - #21440
Conversation
c184053 to
88cac68
Compare
88cac68 to
d80d4a6
Compare
|
@pedroigor @ahus1 Could you please check it? I'm about to send another PRs similar to this: #22438 |
d80d4a6 to
753cc5b
Compare
|
Sorry, one minor refactoring. |
753cc5b to
78541ab
Compare
ahus1
left a comment
There was a problem hiding this comment.
I'm ok to proceed here. Still I'm concerned by the complexity we of the code we have here to make things work. It is a burden for future enhancements, still I don't know a better solution here.
Just approving, will give someone more familiar with Quarkus the change to review this as well.
|
@ahus1 Thanks. For this PR, it might look as it's more complicated than it should be, but it's the starting point for other PRs. I prefer to have it easily manageable. Modularized Keycloak is far away, so I think this approach is suitable for the stuff we want to achieve :)) @vmuzikar Could you please check it? |
|
@vmuzikar @pedroigor Any input on this? |
pedroigor
left a comment
There was a problem hiding this comment.
I also see a bit more complexity for no real benefit. You are basically moving the resolution of ignored artifacts to another place.
Don't you also need to update https://github.com/keycloak/keycloak/blob/main/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java?
The way artifacts are ignored now after one of the latest changes we got from Quarkus, I think we don't even need the option transformer in ClassLoaderPropertyMappers, do we?
|
@pedroigor Thanks for the review
As I mentioned, this baseline PR might look quite complex, but IMO, after providing all these changes within the follow-up PRs, the major complexity would be in the ClassLoader mapper. I think separating the logic behind it is reasonable.
AFAIK, there shouldn't be any changes necessary in the Build command as it works already with the transformed values in the property.
Can you point me to the changes we got from Quarkus? I'm probably not familiar with that as I still think the transformer is necessary there. |
|
@pedroigor Are you ok with this PR? |
78541ab to
94425c6
Compare
|
@pedroigor What would be your suggestion for this? I consider the solution cleaner than handling all the logic in the |
|
I don't have any proposal. That is why I'm not against it. I only think we have now more code rather than everything within the mapper. In terms of testing, we can only catch if the ignored artifacts are correct if we actually run integration tests, as we have for FIPS and the new store. Some errors might happen only when loading classes at runtime. |
| private static final class Builder { | ||
| private final Set<String> finalIgnoredArtifacts; | ||
|
|
||
| public Builder() { | ||
| this.finalIgnoredArtifacts = new HashSet<>(); | ||
| } | ||
|
|
||
| public Builder append(Set<String> ignoredArtifacts) { | ||
| finalIgnoredArtifacts.addAll(ignoredArtifacts); | ||
| return this; | ||
| } | ||
|
|
||
| public Set<String> build() { | ||
| return finalIgnoredArtifacts; | ||
| } |
There was a problem hiding this comment.
This feels to me like a bit overkill. ;) Could be easily replaced e.g. by Stream.of(...).flatMap(Collection::stream).collect(Collectors.toSet());. But not a blocker for me...
Closes #22430
Follow-up of #20976
These changes separate the handling of the ignored artifacts from the property mapper for better manageability of ignored artifacts. As we can consider, we're gonna also exclude artifacts for various JDBC vendors and health/metrics based on the configuration. The property mapper handling would be pretty messy.