fix: ensuring placeholders can be used with --import-realm #33589
Conversation
closes: keycloak#33578 Signed-off-by: Steve Hawkins <[email protected]>
| if (dir == null) { | ||
| throw new IllegalStateException("Import not enabled"); | ||
| } | ||
| ExportImportConfig.setReplacePlaceholders(true); |
There was a problem hiding this comment.
This looks like an odd place to set configuration. Can't this be moved somewhere else (maybe the ExportImportManager)?
There was a problem hiding this comment.
This looks like an odd place to set configuration.
Do you mean you want to get rid of the ExportImportConfig class? That has the responsibility currently of handling all of these system properties.
There was a problem hiding this comment.
Another alternative is to default to always replacing placeholders. I think we didn't do that before just to limit the scope of the changes, but as long as the core folks agree then we can remove both of these calls.
There was a problem hiding this comment.
Do you mean you want to get rid of the ExportImportConfig class? That has the responsibility currently of handling all of these system properties.
No, I meant that I don't think we should be calling ExportImportConfig.setReplacePlaceholders(true); from isImportMasterIncludedAtStartup which is essentially just a getter/checker, it should not modify the configuration IMHO.
Another alternative is to default to always replacing placeholders. I think we didn't do that before just to limit the scope of the changes, but as long as the core folks agree then we can remove both of these calls.
That might be a bit out of scope for now.
There was a problem hiding this comment.
No, I meant that I don't think we should be calling ExportImportConfig.setReplacePlaceholders(true); from isImportMasterIncludedAtStartup which is essentially just a getter/checker, it should not modify the configuration IMHO.
No, it's not great. It's similar to the issues raised in the TODO about undoing the static state changes. We can move it up, but it's not ideal in KeycloakApplication either.
There was a problem hiding this comment.
TBH, it makes more sense to me if it was in KeycloakApplication than in isImportMasterIncludedAtStartup. Feels cleaner.
There was a problem hiding this comment.
Let's just resolve this in a follow-up: #33596 - ideally we should be encapsulating as much as possible the usage of static ExportImportConfig logic.
There was a problem hiding this comment.
Ok, let's do it as a follow-up.
| { | ||
| "realm": "quickstart-realm", | ||
| "enabled": true, | ||
| "enabled": ${TEST_REALM_ENABLED:true}, |
There was a problem hiding this comment.
We're not explicitly testing if the realm is enabled in the ImportAtStartupDistTest but it should be fine as the import would fail if placeholders were disabled.
| if (dir == null) { | ||
| throw new IllegalStateException("Import not enabled"); | ||
| } | ||
| ExportImportConfig.setReplacePlaceholders(true); |
There was a problem hiding this comment.
Ok, let's do it as a follow-up.
…33589) closes: keycloak#33578 Signed-off-by: Steve Hawkins <[email protected]> (cherry picked from commit cb3954f)
…33598) closes: #33578 Signed-off-by: Steve Hawkins <[email protected]> (cherry picked from commit cb3954f)
closes: #33578