-
Notifications
You must be signed in to change notification settings - Fork 3k
Use dev services instead of docker-maven-plugin for Postgresql #51066
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.
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
base: main
Are you sure you want to change the base?
Use dev services instead of docker-maven-plugin for Postgresql #51066
Conversation
|
/cc @gsmet (hibernate-orm) |
This comment has been minimized.
This comment has been minimized.
|
Yes, it's a trick in the source itself :) Originally, the JDBC Extension queried the JDBC URL value at build time to either start or skip the DevService. In cases where the URL was an expansion, this would fail if the expansion is not available at build time. In the original issue, the JDBC URL was set as an expansion in Vault, only available at runtime (see what I mean that we shouldn't be querying runtime configuration at build time? :)) So I've changed that to just query for the name to be available, since we don't care about the value at all when we have to determine if we launch the DevService. To simulate the original issue, the source only sets Lines 20 to 39 in 1243412
So in this case, you get a value at build time, which disables the DevService, and then when runtime starts, there is no value, which causes the message saying that the URL is not set. Testing for this particular situation when using DevServices is mutually exclusive. We can either remove the config source and trust that our code is not querying for the actual config value (by always using |
Thank you for the explanation! So this would be a correct refactoring of the original test? Without this PR, that is. From to |
|
Yes, it doesn't really matter what the expression configuration name is. We just want to ensure that the expression resolution is not available at build time and that we don't try to expand it, as that would cause an error. |
1243412 to
caeda87
Compare
Got it, thanks! I'll switch the property name to make it more clear it's not supposed to work. Since the other property was also used in the application.properties I thought it was supposed to be resolvable. I've reworked the tests and I have something that passes, and I think it still exercises the same things, but I'd value a check from @radcortez. What I've done is use dev services for most of the tests in that project, and added a new test which uses a resource lifecycle manager to start postgres. I also tried |
This comment has been minimized.
This comment has been minimized.
caeda87 to
cf5c0e8
Compare
This comment has been minimized.
This comment has been minimized.
|
Excellent! |
| <!-- <dependency>--> | ||
| <!-- <groupId>org.junit.platform</groupId>--> | ||
| <!-- <artifactId>junit-platform-suite</artifactId>--> | ||
| <!-- <scope>test</scope>--> | ||
| <!-- </dependency>--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed? Or maybe we need a comment to explain why it's here and commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's leftover from when I tried to make use a junit suite to pre-start the database. Oops. Will fix and squash, thanks!
|
I've address your review comments (thanks!). I think I've got something valid on the readme? I also noticed a similar issue on one of the reactive readmes so I've updated it as well. I didn't squash so you could see just those changes. |
Status for workflow
|
Partial resolution of #44124.
As with #51059, I left the
extensions/*project using an independently-managed container, since the test exercisesapplication.propertiesa lot.I'm marking this as a draft, because I had a failure I can't explain. I'm hoping @radcortez and @yrodiere might be able to help. I don't know if it's a bug, or if I'm asking the test to do something that's not supported. The
jpa-postgresqlproject fails withThe reason seems to be related to the config override,
OverrideJdbcUrlBuildTimeConfigSource, which sets aquarkus.datasource.jdbc.url, to reproduce #16123. If I disable that override by deletingsrc/test/resources/META-INF/services/org.eclipse.microprofile.config.spi.ConfigSource, the test all passes. With the override, I'd expect a connection failure, becausequarkus.datasource.jdbc.urlwould disable the dev service, and there's nothing listening at the hardcoded endpoint. But instead I get an error saying thequarkus.datasource.jdbc.urlproperty isn't set. It's like the dev service processor sees it as set, so doesn't start anything, but theAgroalRecordersees it as not set. Maybe that's expected because of when the reading and writing of the config happens, but it makes me wonder if the test is testing what it's supposed to be testing. As far as I can tell, the override was just setting the same value as was set inapplication.propertiesalready.