8000 consolidating profile logic and moving reaug related logic by shawkins · Pull Request #38514 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

consolidating profile logic and moving reaug related logic - #38514

Merged
vmuzikar merged 1 commit into
keycloak:mainfrom
shawkins:hostname_strict_https
Jul 2, 2025
Merged

consolidating profile logic and moving reaug related logic#38514
vmuzikar merged 1 commit into
keycloak:mainfrom
shawkins:hostname_strict_https

Conversation

@shawkins
@shawkins shawkins commented Mar 28, 2025 •
edited
Loading
Copy link
Copy Markdown
Contributor

This is a follow up to #38236 - after trying to add a test via PicocliTest for the hostname-strict-https check, it was clear that our current way of emitting the warning handling profiled properties wasn't consistent.

also aggregated the various places that were setting the profile to a single place

closes: #38581

@shawkins
shawkins requested review from a team as code owners March 28, 2025 01:23
@shawkins
shawkins force-pushed the hostname_strict_https branch from 8128ee3 to 88920db Compare March 28, 2025 03:13
@shawkins
shawkins marked this pull request as draft March 28, 2025 03:15
@shawkins
shawkins force-pushed the hostname_strict_https branch 2 times, most recently from 831063c to cbeee84 Compare March 28, 2025 15:46
@shawkins shawkins mentioned this pull request Mar 28, 2025
Merged
@shawkins shawkins changed the title fix: removing the use of hostname-strict-https mistakenly brought back consolidating profile logic Mar 28, 2025
@shawkins
shawkins force-pushed the hostname_strict_https branch from cbeee84 to 682abcf Compare April 1, 2025 12:05
@shawkins
shawkins marked this pull request as ready for review April 1, 2025 12:05
@shawkins shawkins added the status/hold PR should not be merged. On hold for later. label Apr 1, 2025
@shawkins
shawkins force-pushed the hostname_strict_https branch from 682abcf to cea0704 Compare April 1, 2025 12:10
@shawkins
shawkins marked this pull request as draft April 2, 2025 12:38
@shawkins shawkins changed the title consolidating profile logic consolidating profile logic and moving reaug related logic Apr 2, 2025
@shawkins
shawkins force-pushed the hostname_strict_https branch 2 times, most recently from 3f34f52 to aab109d Compare April 2, 2025 17:49
@shawkins
shawkins commented Apr 2, 2025
Copy link
Copy Markdown
Contributor Author

@keycloak/cloud-native this stops short of what is described in #38581 (comment) - while the changes don't lead to a reduction in line count, they do start to make Picocli less of a monolith and aggregate the code in hopefully a more understandable way.

The downsides at this point are:

  • rebuild checks are slightly more expensive - as they need picocli execution to run. On the other hand we get to remove quite a few places where we have potentially error prone scanning of the cli options.
  • the help logic must not emit anything when it's a rebuild check, so there's a new check for this with empty args.

Both of these concerns could be removed if we adopt #38581 (comment)

Also we'll ideally eventully pare back the usage ConfigArgsConfigSource.parseConfigArgs more - the ParseResult we're getting from picocli is enough give us the unmatched parameters, which we can then treat as our PropertyMapper / cli options - with no guess work as to what are command names / pico cli options.

@shawkins
shawkins force-pushed the hostname_strict_https branch from 3513998 to 3d36dad Compare April 2, 2025 20:16
@shawkins
shawkins marked this pull request as ready for review April 2, 2025 20:57
@shawkins
shawkins force-pushed the hostname_strict_https branch from 3d36dad to f91aa2a Compare April 3, 2025 11:52
@keycloak-github-bot
keycloak-github-bot Bot commented Apr 3, 2025
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.actions.TermsAndConditionsTest#termsAccepted

Keycloak CI - Base IT (1)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.actions.TermsAndConditionsTest#termsDisabled

Keycloak CI - Base IT (1)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Apr 3, 2025
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@shawkins shawkins mentioned this pull request Apr 3, 2025
Closed
1 of 2 tasks
@shawkins
shawkins force-pushed the hostname_strict_https branch from f91aa2a to 7488c80 Compare April 3, 2025 15:57
@shawkins shawkins mentioned this pull request Apr 3, 2025
Closed
1 of 2 tasks
shawkins
shawkins commented Jun 19, 2025
View reviewed changes
@shawkins shawkins left a comment
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mabartos thank you for the review, made some refinements based upon your comments.

@shawkins
shawkins force-pushed the hostname_strict_https branch from 16ec5e0 to bf9c3ac Compare June 19, 2025 15:31
keycloak-github-bot[bot]
keycloak-github-bot Bot 8000 reviewed Jun 19, 2025
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@keycloak-github-bot
keycloak-github-bot Bot commented Jun 19, 2025
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.cluster.RealmInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

@keycloak-github-bot keycloak-github-bot Bot mentioned this pull request Jun 23, 2025
Closed
mabartos
mabartos previously approved these changes Jun 24, 2025
View reviewed changes
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shawkins Great job! LGTM - some of my comments are only nitpicks and do not need to be necessarily addressed. Can be potentially done in follow-up tasks later. Thanks!

@vmuzikar @Pepo48 Do you plan to look at it? I think it might be merged.

Comment thread quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractCommand.java Resolved
@keycloak-github-bot
keycloak-github-bot Bot commented Jun 24, 2025
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.cluster.RealmInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Jun 24, 2025
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

mabartos
mabartos previously approved these changes Jun 24, 2025
View reviewed changes
vmuzikar
vmuzikar reviewed Jul 1, 2025
View reviewed changes
@vmuzikar vmuzikar left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shawkins Sorry for the late feedback. Added some comments mostly about namings/javadoc.

Comment thread quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractCommand.java Resolved
Comment thread ...kus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractStartCommand.java Resolved
Comment thread quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractCommand.java Resolved
@shawkins shawkins removed the status/hold PR should not be merged. On hold for later. label Jul 1, 2025
@shawkins
shawkins requested review from a team as code owners July 1, 2025 17:08
@shawkins
fix: consolidating profile logic and refactoring Picocli
634a4c7 
moves more logic onto the commands

closes: keycloak#38581

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins
shawkins force-pushed the hostname_strict_https branch from 9b370fe to 634a4c7 Compare July 1, 2025 17:12
@shawkins
shawkins commented Jul 1, 2025
Copy link
Copy Markdown
Contributor Author

Fully rebased.

mabartos
mabartos approved these changes Jul 2, 2025
View reviewed changes
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shawkins Still LGTM, thanks!

vmuzikar
vmuzikar approved these changes Jul 2, 2025
View reviewed changes
@vmuzikar vmuzikar left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the updates!

@vmuzikar
vmuzikar merged commit 9dff965 into keycloak:main Jul 2, 2025
@ryanemerson ryanemerson mentioned this pull request Sep 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar vmuzikar approved these changes
@mabartos mabartos mabartos approved these changes
@Pepo48 Pepo48 Awaiting requested review from Pepo48
+1 more reviewer
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/cloud-native team/core-clients team/core-iam

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consolidate profile logic

3 participants

@shawkins @mabartos @vmuzikar
0