QUARKUS-1490: Use BuildItem to enable use of TestContainers shared network

[manstis]

Following the discussion on Zulip and JIRA this PR corrects the erroneous attempt to control build time behaviour with properties.

[manstis]

@geoand FYI.

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building e19bb68

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	Build	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/datasource/deployment-spi 
! Skipped: devtools/bom-descriptor-json docs extensions/agroal/deployment and 126 more

📦 extensions/datasource/deployment-spi

✖ Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.17.0:validate (default) on project quarkus-datasource-deployment-spi: File '/home/runner/work/quarkus/quarkus/extensions/datasource/deployment-spi/src/main/java/io/quarkus/datasource/deployment/spi/DevServicesUseTestContainersSharedNetworkBuildItem.java' has not been previously formatted. Please format file and commit before running validation!

[geoand]

I just remembered... We already have DevServicesSharedNetworkBuildItem, does that not fit your needs?
If so, then this PR should just remove the configuration value that was added in the previous PR

[manstis]

I just remembered... We already have DevServicesSharedNetworkBuildItem, does that not fit your needs? If so, then this PR should just remove the configuration value that was added in the previous PR

@geoand I must admit DevServicesSharedNetworkBuildItem had me fooled and it may well be that what you describe is its original intention; however looking at Quarkus' extensions it is used in a myriad of different ways some as you'd expect and others not. For example (all examples are before I started to muddle the waters recently):-

The good
InfinispanDevServiceProcessor. This appears to use it as you describe; adding the container to the shared network and creating URLs to expose the service either to localhost or the internal Docker network.

The bad
DevServicesKafkaProcessor. This always joined the shared network and DevServicesSharedNetworkBuildItem only affected the URLs at which the service was visible.

The ugly
PostgresqlDevServicesProcessor never joined the shared network and DevServicesSharedNetworkBuildItem only affected the URLs at which the service was visible.

I did point out the behaviour of DevServicesSharedNetworkBuildItem is at best inconsistent and and worst confused in my original PR but nobody seemed that concerned so I went with what has now become my own BuildItem to provide the functionality I require: that is, to have containers launched by Quarkus using TestContainers to join a single shared network.

If the purpose of DevServicesSharedNetworkBuildItem is indeed to perform this function then I am happy to refactor this PR, remove my new BuildItem and fix use of DevServicesSharedNetworkBuildItem throughout Quarkus. I know the usage in DevServicesKafkaProcessor has proliferated into kogito (I suspect we copied and pasted service instantiation and invocation from you guys!) so whatever is decided should be communicated in release notes.

Happy to help, but will need to be told what you need doing 😃

[geoand]

Understood and sorry for the confusion.

The existing build item we have is the only one that should be used and it should be consistent throughout - meaning that it should force the launched container onto the shared network.

We should get rid of the configuration property and do away with the extra build item added in this PR.

Does that make sense?

[manstis]

@geoand Yes, sure. No problem.

I'll make the necessary changes to make the existing BuildItem work as expected consistently throughout Quarkus. It should arguably become a MultiBuildItem as it can now be created in two places (my extension and when integration tests run) but I'll try to keep it as a SimpleBuildItem to avoid breaking changes. There must be a way I can prevent my extension from creating it if it's being ran inside an integration test.

[geoand]

It should arguably become a MultiBuildItem

Indeed! If it doesn't become one, things will fail.

Thanks a lot and sorry again for not catching this earlier

[manstis]

@geoand I've updated this PR and I now re-use the existing DevServicesSharedNetworkBuildItem from my extension to enable Quarkus to re-use TestContainers shared network. I also tidied up all uses of DevServicesSharedNetworkBuildItem in Quarkus' extensions. Great.........

I did however have to add a DevServicesLocalNetworkBuildItem though... When shared networking is enabled Kafka is only visible to the shared network. My application using my extension also uses DevServices' Kafka (Redpanda) service. Using this new BuiltItem I can also make the service accessible from both my local- and the shared networks.

DevServicesLocalNetworkBuildItem could be re-used from other services; but to be honest: (1) I have no use of other services locally, (2) DataSources look particularly interesting... as you no doubt know they replace DataSource URLs defined by the application with ones that are either suitable for local->Docker networking or Docker->Docker "shared networking". I can't see how DevServicesLocalNetworkBuildItem would be of any use in those scenarios. That.. if needed.. can wait for another day.

Anwyay, at the risk of rambling on further this seems a reasonable approach: maintaining the purpose and function of of DevServicesSharedNetworkBuildItem whilst adding the ability for services to also be exposed locally with an analogous approach. WDYT?

[geoand]

Thanks for the update.

I'll have to take a closer look tomorrow on Monday since there are too many things going in parallel that need my attention.

[geoand]

Although I didn't look at the details (I'll do that as soon as time permits), I am +1 for the approach.

Thanks for your patience on this while I have been unable to focus on the issue properly :)

[geoand]

Thanks!

[geoand]

Coming back to this after encountering a different problem: Why was this call removed? Do you remember?
It seems wrong to me...
Same thing for the other dev services where the same change was made.

[manstis]

It was made consistent with the other DevServices and enabled here if useSharedNetwork is enabled.

[manstis]

Apicurio was always on the shared network; even if useSharedNetwork was false... perhaps it needed to be that way, perhaps it did not.. but there was nothing to suggest either way.. so yes use of useSharedNetwork was made "consistent" across all DevServices..... like I said yesterday, looking Quarkus is both fun and dangerous!

[geoand]

But that part doesn't call withNetwork or setNetworkAliases unless I am mistaken. Am I missing something?

[geoand]

Ah sorry, my bad - I see where the call was moved to.

False alarm :)

[manstis]

Yes, it's a "nice" method call isn't it... took me a while before I stepped into it too!

[geoand]

👍🏼

[rsvoboda]

@manstis do you have any pointer where this gets used?

PR doesn't contain any test, also no docs update.

[geoand]

@rsvoboda DevServicesSharedNetworkBuildItem is used in all the dev-services modules (for example quarkus-devservices-postgresql) and is therefore exercised in the various tests that use dev services.

Furthermore, tests like container-build-jib-with-postgresql are the ones that actually test this

[manstis]

@manstis do you have any pointer where this gets used?

PR doesn't contain any test, also no docs update.

I think @geoand has advised appropriately.

[manstis]

@rsvoboda The change I made to DevServicesSharedNetworkBuildItem was to allow extensions to activate DevServices services. In kiegroup we have an extension that wanted to re-use DevServices' PostgreSQL support - rather than spinning up it's own instance etc. In our extension we build DevServicesSharedNetworkBuildItem that then allows us to use DevServices PostgreSQL instance.

[manstis]

@rsvoboda We also use DevServices RedPanda instance/support from our extension too.

Similarly DevServicesSharedNetworkBuildItem exposes this for us on Quarkus's "shared" network.

[rsvoboda]

Thanks @geoand and @manstis