8000 fix: "narrow" fix to persist build time spi options by shawkins · Pull Request #33979 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
/ keycloak Public

fix: "narrow" fix to persist build time spi options #33979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

fix: "narrow" fix to persist build time spi options #33979

wants to merge 4 commits into from

Conversation

shawkins
Copy link
Contributor
@shawkins shawkins commented Oct 16, 2024 •
edited
Loading

closes: #33902

Radical simplification from the changes proposed in #33760

@shawkins
fix: narrow fix to persist build time spi options
b5d160b 
closes: keycloak#33902

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
vmuzikar
vmuzikar reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

Changes LGTM but I think we're missing an integration test aiming for this particular fix.

@shawkins
Copy link
Contributor Author

Changes LGTM but I think we're missing an integration test aiming for this particular fix.

This draft was to see if there was the same problem with all the other test failures as the other pr - and there are. The arquillian tests are seemingly reliant upon the behavior of not persisting the build time spi options. Haven't figured out why yet.

@shawkins
Copy link
Contributor Author

@vmuzikar @mabartos @Pepo48 the issue appears to be related to the "restart" handling in the KeycloakQuarkusServerDeployableContainer. Before the container successfully starts, it's deleting the data dir. On the next startup it's expecting to "restart" - meaning triggering a build. This no longer happens after the change in this pr. What I'm not understanding immeditately is the relationship between running build and the database being deleted - what build time side effects are there related to database setup?

@shawkins shawkins marked this pull request as ready for review October 17, 2024 14:57
@shawkins shawkins requested review from a team as code owners October 17, 2024 14:57
@shawkins shawkins marked this pull request as draft October 17, 2024 14:57
@vmuzikar
Copy link
Contributor

I think this worked just by "accident". Even the original PR that introduced it is claiming to be:

Deleting data dir between tests run

Which is simply not true. The data dir is deleted only for the first test run in the whole testsuite.

@pedroigor Are we perhaps missing something. :) Do you remember the reasoning behind it?

pedroigor
pedroigor previously approved these changes Oct 18, 2024
View reviewed changes
@pedroigor
Copy link
Contributor

@vmuzikar Not sure. What you mean by working by accident?

@shawkins
Copy link
Contributor Author

After talking things over with @pedroigor we determined that the arquillian tests are reliant upon the profile.properties for manipulating features, which is effectively broken once we persist the spi build time options. I will take a pass at removing the use of profile.properties and --optimized from the arquillian tests so that we can just rely on the real auto-build.

@keycloak-github-bot
Copy link

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.javascript.JavascriptAdapterTest#testSilentCheckSso

Keycloak CI - Base IT (5)

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 Oct 18, 2024
View reviewed changes
Copy link
@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

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

8000

Unreported flaky test detected, please review

@shawkins
Copy link
Contributor Author
shawkins commented Oct 18, 2024 •
edited
Loading

@vmuzikar @pedroigor @Pepo48 @mabartos updated with initial changes to just use auto-builds. There will likely be a few additional clean ups, then some quarkus integration test to fully confirm we're persisting things.

What we could do differently:

  • keep --optimized / log scraping - my assumption was there is only minimal value in skipping the auto-build checks, but if anyone feels differently we can instead react to the logs and signal dropping the --optimized
  • I don't like the usage of the profile.properties the quarkus server - so it was removed from the runtime here. However how we specify features enabled / disabled in quarkus via a string list isn't ideal for testing - if we could do this directly from individual properties in the config, it would be easier. I had started down that path a while ago with versioned features:

    keycloak/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/Environment.java

    Line 245 in 47a7d9c

    profile = Profile.configure(new QuarkusProfileConfigResolver(), new PropertiesProfileConfigResolver(QuarkusProfileConfigResolver::getConfig), new PropertiesFileProfileConfigResolver());
    - but abandoned it after seeing how involved the test changes would be. So I'm removing the usage of the PropertiesProfileConfigResolver here also because it's not doing anyting - but we could revisit that. Alternatively we could make changes to both the KeycloakProcessor and Picocli (or just in one place with fix: makes the db setting require an explicit setting in prod #33760) to persist / check the profile.properties.

@keycloak-github-bot keycloak-github-bot bot mentioned this pull request Oct 19, 2024
Closed
@shawkins shawkins force-pushed the iss33902 branch 6 times, most recently from 916befc to 76cf7d0 Compare October 20, 2024 11:41
@shawkins
refining the test suite to simply use auto-builds
0ee953d 
also removed profile.properties support from quarkus

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins
avoid parsing ambiguities of incomplete encoding schemes
6c91bf0 
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@pedroigor
Copy link
Contributor
  • keep --optimized / log scraping - my assumption was there is only minimal value in skipping the auto-build checks, but if anyone feels differently we can instead react to the logs and signal dropping the --optimized

IMO, the key point is to avoid increasing test execution time significantly, ideally reduce it. If auto-build is faster, the same, or slightly worse, I think it is worth relying on auto-builds (and the auto-build check) to make this simpler.

@shawkins
moving properties out of keycloak.conf
c977bb0 
adding a direct test of spi auto-build

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins shawkins changed the title fix: narrow fix to persist build time spi options fix: "narrow" fix to persist build time spi options Oct 21, 2024
@shawkins shawkins marked this pull request as ready for review October 21, 2024 12:10
@shawkins shawkins requested review from a team as code owners October 21, 2024 12:10
@shawkins
Copy link
Contributor Author

Summary of changes here:

  • persist build-time spi options
    • this requires excluding the runtim options from being picked up by the default value source
  • change the arquillian test suite to use auto-builds (mentioned more in-depth in the last comment)
  • I believe there are further improvements that could be made here
  • correct how we track cli options - arguments beginning or ending with ;, or containing ;; will parse incorrectly. We were affected by this with some database urls

@shawkins shawkins closed this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

@vmuzikar vmuzikar Awaiting requested review from vmuzikar

@pedroigor pedroigor Awaiting requested review from pedroigor

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.

Not persisted config settings prevent server start
3 participants
@shawkins @vmuzikar @pedroigor
0