+
Skip to content

test: Provide test cases for datasources ENV vars handling #40760

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
Merged
merged 1 commit into from
Jul 1, 2025
Merged

test: Provide test cases for datasources ENV vars handling #40760

merged 1 commit into from
Jul 1, 2025

Conversation

mabartos
Copy link
Contributor

@mabartos mabartos commented Jun 27, 2025 •
edited
Loading

Related-to: #40387

We should provide some simple test cases for ENV var handling for additional datasources, as we're lacking them.

@shawkins Could you please check it? Thanks

@mabartos mabartos requested review from a team as code owners June 27, 2025 14:06
@mabartos mabartos marked this pull request as draft June 27, 2025 14:09
mabartos
mabartos commented Jun 27, 2025
View reviewed changes
...e/src/test/java/org/keycloak/quarkus/runtime/configuration/DatasourcesConfigurationTest.java
Comment on lines +413 to +414
"quarkus.datasource.\"my-store\".db-kind", "mariadb",
"quarkus.datasource.\"my.store\".db-kind", "mariadb"
Copy link
Contributor Author

@mabartos mabartos Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Even though we're able to obtain the configuration for both variants (my-store and my.store), the named datasource in Quarkus is my.store. See https://github.com/keycloak/keycloak/pull/40760/files#diff-161196ef0d1fee87940b57c35aa7d52ccc6da78c93eed92064e51483e047e101R36

It would be probably better to have the - (minus) char the default for datasources, but probably not a big deal when we document it.

cc: @shawkins

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though we're able to obtain the configuration for both variants (my-store and my.store)

That is a side effect of the quarkus env config source. Anything that could map to the env form can be used. Only the my.store form is the canonical name that is advertised to quarkus (my!store). It would be fine to remove the assertions looking for the my-store form.

It would be probably better to have the - (minus) char the default for datasources

Yes, it would be best to do that for everything that is not logging. It doesn't make sense to default to the quarkus convention when we use '-'. I was going to add this for the spi work, but that didn't move forward, so it can be done here if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be best to do that for everything that is not logging. It doesn't make sense to default to the quarkus convention when we use '-'. I was going to add this for the spi work, but that didn't move forward, so it can be done here if you want.

@shawkins Agree. So, could you please provide such changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be best to do that for everything that is not logging. It doesn't make sense to default to the quarkus convention when we use '-'

+1
Though, it would be considered a breaking change. We're already releasing the initial additional datasources support in 26.3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though, it would be considered a breaking change. We're already releasing the initial additional datasources support in 26.3

@vmuzikar Depends on how you look at it. Understand that we already have these options for it, but we don't state anywhere that it's supported, and we don't even have the documentation for it yet. So maybe it should not be a big deal.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mabartos Good point that there's no docs. Though it's included in --help and NOT marked as a preview or something. We should've probably done that, I didn't realize it. :/

@mabartos mabartos marked this pull request as ready for review June 27, 2025 14:12
@mabartos
test: Provide test cases for datasources ENV vars handling
3fd659a 
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
@mabartos mabartos mentioned this pull request Jun 27, 2025
Open
4 tasks
@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.cluster.RealmInvalidationClusterTest#crudWithFailover

Keycloak CI - Store IT (postgres)

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 27, 2025
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.

Unreported flaky test detected, please review

@mabartos mabartos requested a review from shawkins July 1, 2025 09:17
vmuzikar
vmuzikar approved these changes Jul 1, 2025
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.

I'd still proceed with the PR as is. We should address the . vs - in a separate PR.

@mabartos
Copy link
Contributor Author

mabartos commented Jul 1, 2025

I'd still proceed with the PR as is. We should address the . vs - in a separate PR.

@vmuzikar Agree.

@mabartos mabartos merged commit 41dcad5 into keycloak:main Jul 1, 2025
80 checks passed
@shawkins shawkins mentioned this pull request Jul 1, 2025
Closed
shawkins pushed a commit to shawkins/keycloak that referenced this pull request Jul 1, 2025
@mabartos @shawkins
test: Provide test cases for datasources ENV vars handling (keycloak#…
0c4cbd6 
…40760)

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
oculos pushed a commit to unioslo/keycloak that referenced this pull request Jul 4, 2025
@mabartos @oculos
test: Provide test cases for datasources ENV vars handling (keycloak#…
3368fcd 
…40760)

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
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

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

@shawkins shawkins Awaiting requested review from shawkins

Assignees
No one assigned
Labels
flaky-test team/cloud-native
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mabartos @shawkins @vmuzikar
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载