Ciam 6580 issue 471 hiearchical groups #472
Conversation
eatikrh
force-pushed
the
CIAM-6580-issue-471-hiearchical-groups
branch
from
b5363b3
to
6814ccc
Compare
eatikrh
force-pushed
the
CIAM-6580-issue-471-hiearchical-groups
branch
from
6814ccc
to
539129e
Compare
There was a problem hiding this comment.
Hi @eatikrh - thank you for providing this PR. I pushed one review commit with aligns some code formatting and avoids star imports.
I also found that
name1.split(GROUP_NAME_SEPARATOR);
might not lead to the results you want to see, as the parameter is a regex. Passing in a dot (.) would match any character. I've suggested
name1.split(Pattern.quote(GROUP_NAME_SEPARATOR));
instead.
There is also one unused parameter. Please have a look.
Once you're ok with the code, please have a team member of your team test this code if it delivers the results you want to see (I've seen you've discussed it with @mposolda in the parent issue), and then let them add a comment/review here. Then it can be merged by one of the maintainers of this project.
Thanks!
| @@ -972,12 +979,49 @@ private void createClients(RealmContext context, Task task, KeycloakSession sess | |||
| task.debug(logger, "Created %d clients in realm %s", context.getClientCount(), context.getRealm().getName()); | |||
| } | |||
|
|
|||
| private void createGroups(RealmContext context, int startIndex, int endIndex, KeycloakSession session) { | |||
| private String getGroupName(boolean hiearchicalGroups, int hierarchyDepth, int countGroupsAtEachLevel, String prefix, int currentCount) { | |||
There was a problem hiding this comment.
The parameter hierarchyDepth is not used, please review/test the code.
There was a problem hiding this comment.
Hi @ahus1
Thank you for the review and the fixes. Yes "." is tricky in regex. I removed the unused parameter and a typo.
|
@mposolda can please also take a look at this PR? Also do we have a Code Style template for IntelliJ? |
|
@eatikrh - there is no code style for this ... once this PR is merged, I'll see that we add a minimal Checkstyle configuration. |
There was a problem hiding this comment.
@eatikrh Thanks for the PR! It looks great. I've added one comment inline regarding some docs for new parameters.
Also it can be ideal to add some documentation with examples into this file https://github.com/keycloak/keycloak-benchmark/blob/22799bc65e39f8e353f2fb57a6938d051682b015/doc/dataset/modules/ROOT/pages/using-provider.adoc . Is it possible to add some new section like Create many groups with some examples for hierarchical and non-hierarchical groups?
dataset/src/main/java/org/keycloak/benchmark/dataset/config/DatasetConfig.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank you for the changes. I updated the docs a little as I am a fan of AsciiDoc description lists.
This PR creates hierarchical groups using the configuration parameters: groups-count-each-level, groups-hierarchy-depth, groups-with-hierarchy. Boolean parameter groups-with-hierarchy is "false" by default. With the default values it works as before.
With groups-with-hierarchy=true, it ignores groups-per-realm parameter and creates the group tree structure as defined by the other parameters. This will be groups-count-each-level ^ groups-hierarchy-depth.
The implementation honors groups-per-transaction.
We adopted a subgroup naming convention that uses "." in the group names which allows finding the parent group even if it was created in a previous transaction.
Closes #471