8000 Persisted config source not loading properties at runtime by pedroigor · Pull Request #12157 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Persisted config source not loading properties at runtime - #12157

Merged
abstractj merged 2 commits into
keycloak:mainfrom
pedroigor:issue-12156
May 25, 2022
Merged

Persisted config source not loading properties at runtime#12157
abstractj merged 2 commits into
keycloak:mainfrom
pedroigor:issue-12156

Conversation

@pedroigor
@pedroigor pedroigor commented May 23, 2022
Copy link
Copy Markdown
Contributor

Closes #12156

@pedroigor
pedroigor requested a review from andreaTP May 23, 2022 23:26
@pedroigor pedroigor mentioned this pull request May 23, 2022
Closed
@pedroigor
pedroigor commented May 24, 2022
Copy link
Copy Markdown
Contributor Author

Looks like nexus is unavailable.

DGuhr
DGuhr suggested changes May 24, 2022
View reviewed changes
@DGuhr DGuhr left a comment •
edited
Loading
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.

@pedroigor the fix itself in the codebase LGTM, I wasn't sure if checking for the ClassLoader instance was right at all, wanted to discuss with you. Seems not :)

For the testing enhancements:

  • When no additional test dependencies should be added (as per this comment), why you also add 2? Easily achievable using only the Testcontainers library we have, and GenericContainer with the appropriate images (ref), but a bit more boilerplate involved (setting DB user, pw etc using withEnv or sth), so I thought add the dependency, only for test for sure, to avoid this boilerplate. Feel free to leave them, as it is what I did essentially, it's just that I don't get that comment then ;)
  • pls incorporate my tests in this PR in order to make it approvable. Looking at your code, you set db using dist.setProperty() so it would be available from non-persistent source in the properties file we generate unless we have a removeProperty that we don't have or we use ReInstall.BEFORE_TEST that makes no sense.
    To make it more concise: This test that I introduced to make sure the db prop is read from persisted properties would fail with your current PR I guess, bc. db is actually read from the properties file in the temp kc installation instead of being read from the persisted properties. This would lead to the subsequent tests actually succeeding (see proposed workaround here), but is not the use case we want to test => using a pre-optimized serverimage without the db set anywhere but in persisted props.
  • w.r.t. "adding functionality for additional databases": I avoided this to not add unnecessary complexity until it is needed. But when it works, feel free to add it for mariadb, too, but then there's the question why not add it for all DBs? Imo this is a point for an extra ticket or should be incorporated for additional DBs whenever it is needed by a specific use case. My PR introduced "just enough" code to fix the issue. I used postgres bc. it is the only db the operator also supports, and one of the two dbs we want to go with later on as first-class, so imo this should be our go-to db for testing. Also, the problem was not really db-specific, but configuration and build/runtime behaviour specific so one test for one db should suffice w.r.t. test execution 8000 times. That said, when it works, why not..

*hushs back to PTO ;)

@pedroigor
pedroigor commented May 24, 2022
Copy link
Copy Markdown
Contributor Author

@pedroigor the fix itself in the codebase LGTM, I wasn't sure if checking for the ClassLoader instance was right at all, wanted to discuss with you. Seems not :)

For the testing enhancements:

  • When no additional test dependencies should be added (as per this comment), why you also add 2? Easily achievable using only the Testcontainers library we have, and GenericContainer with the appropriate images (ref), but a bit more boilerplate involved (setting DB user, pw etc using withEnv or sth), so I thought add the dependency, only for test for sure, to avoid this boilerplate. Feel free to leave them, as it is what I did essentially, it's just that I don't get that comment then ;)

Sorry, I meant you don't need to set a version if you have a transient dependency. The way you did will force us to change versions between Quarkus upgrades.

Keep in mind that we should leverage dependencies (and versions) from Quarkus as much as possible

  • pls incorporate my tests in this PR in order to make it approvable. Looking at your code, you set db using dist.setProperty() so it would be available from non-persistent source in the properties file we generate unless we have a removeProperty that we don't have or we use ReInstall.BEFORE_TEST that makes no sense.
    To make it more concise: This test that I introduced to make sure the db prop is read from persisted properties would fail with your current PR I guess, bc. db is actually read from the properties file in the temp kc installation instead of being read from the persisted properties. This would lead to the subsequent tests actually succeeding (see proposed workaround here), but is not the use case we want to test => using a pre-optimized serverimage without the db set anywhere but in persisted props.

I've added the test to check show-config and run all tests without the db option set in the configuration file.

  • w.r.t. "adding functionality for additional databases": I avoided this to not add unnecessary complexity until it is needed. But when it works, feel free to add it for mariadb, too, but then there's the question why not add it for all DBs? Imo this is a point for an extra ticket or should be incorporated for additional DBs whenever it is needed by a specific use case. My PR introduced "just enough" code to fix the issue. I used postgres bc. it is the only db the operator also supports, and one of the two dbs we want to go with later on as first-class, so imo this should be our go-to db for testing. Also, the problem was not really db-specific, but configuration and build/runtime behaviour specific so one test for one db should suffice w.r.t. test execution times. That said, when it works, why not..

There is no additional complexity, we are just leveraging the capabilities available from the test suite to also allow bootstrapping a container when running distribution tests. No need to manually create containers within tests but just use the annotation we already have. We should try to enhance the testsuite whenever we have a chance and here we are now able to run tests against the distribution using a database (in a generic way).

*hushs back to PTO ;)

Let's talk more once you're back. The latest changes should be enough to introduce your test scenario.

@pedroigor @DGuhr
Persisted config source not loading properties at runtime 8000
6cb2e20 
Closes keycloak#12156

Co-authored-by: Dominik Guhr <dguhr@redhat.com>
andreaTP
andreaTP reviewed May 24, 2022
View reviewed changes
@andreaTP andreaTP 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.

Nice work! Added a few comments

Comment thread quarkus/tests/integration/src/main/java/org/keycloak/it/junit5/extension/DatabaseContainer.java Resolved
Comment thread quarkus/tests/integration/src/main/java/org/keycloak/it/junit5/extension/WithDatabase.java Resolved
Comment thread quarkus/tests/integration/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java Resolved
Comment thread quarkus/tests/integration/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java Resolved
Comment thread quarkus/tests/integration/src/test/java/org/keycloak/it/storage/database/BasicDatabaseTest.java Resolved
Comment thread quarkus/tests/integration/src/test/java/org/keycloak/it/storage/database/BasicDatabaseTest.java Resolved
Comment thread quarkus/tests/integration/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java Resolved
@andreaTP
andreaTP commented May 24, 2022
Copy link
Copy Markdown
Contributor

Side note: the Operator CI might be fixed by those changes:
#11642

andreaTP
andreaTP approved these changes May 25, 2022
View reviewed changes
@andreaTP andreaTP left a comment
Copy link
Copy Markdown
Contributor

LGTM

@abstractj
abstractj merged commit 6156272 into keycloak:main May 25, 2022
This was referenced May 26, 2022
Closed
Closed
@stianst stianst added this to the 18.0.1 milestone Jun 7, 2022
DGuhr pushed a commit to DGuhr/keycloak that referenced this pull request Jun 7, 2022
@pedroigor @stianst
Persisted config source not loading properties at runtime (keycloak#1…
769661e 
…2157)

Co-authored-by: Dominik Guhr <dguhr@redhat.com>
 Conflicts:
	.github/workflows/ci.yml
	quarkus/tests/integration/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abstractj abstractj abstractj approved these changes
+2 more reviewers
@DGuhr DGuhr DGuhr requested changes
@andreaTP andreaTP andreaTP approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

18.0.1

Development

Successfully merging this pull request may close these issues.

Persisted config source not loading properties at runtime

5 participants

@pedroigor @andreaTP @abstractj @DGuhr @stianst
0