Skip to content
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

Enable dev services for all named mongodb-client connections #26011

Merged
merged 1 commit into from
Jun 30, 2022
Merged

Enable dev services for all named mongodb-client connections #26011

merged 1 commit into from
Jun 30, 2022

Conversation

evanchooly
Copy link
Member

fixes #25278

@evanchooly
Copy link
Member Author

Interesting follow up question here (and perhaps a change of direction): in order to create a named client, there has to be something in application.properties referencing that name. That something can't use connection-string or devservices won't start something up for it. Now, generally speaking, the likelihood of not having any configuration for the other clients is slim. with prod entries there will be at least a connection-string and credentials and the like so that's not a problem. But if the difference in environments is only that connection and credentials and everything else is defaulted (fairly likely) having a dummy entry in the config just to surface that named client name seems a bit lame. Could we infer the existence of a named client by discovering uses of @MongoClientName and backfilling that client using only that name and the default values of the config? In essence, lazy init'ing the named client and letting devservices have its usual go at it.

@geoand
Copy link
Contributor

geoand commented Jun 9, 2022

Could we infer the existence of a named client by discovering uses of @MongoClientName and backfilling that client using only that name and the default values of the config? In essence, lazy init'ing the named client and letting devservices have its usual go at it

Probably, but I am thinking that it might break stuff, so I would like to avoid it.

import io.smallrye.common.annotation.Blocking;

@Path("/books")
@Blocking
public class BookResource {

@Inject
@MongoClientName("parameter-injection")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need the test to have both named and non-named clients

Copy link
Contributor

Choose a reason for hiding this comment

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

+1
Do we have a way to access the devservices information from a test to check that 2 devservices has been started ? The default and the named one ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default client is injected in PojoResource

Copy link
Member Author

Choose a reason for hiding this comment

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

we can have only one unnamed client and that will have to be done either via dev services or MTR. I pushed a different config that uses MTR for the default and a named one and two named clients using dev services. This covers, i think, all the combos except for default via dev services. I've verified this "manually" but because the config has to be this or way that, it's not "permanently" configured this way.

@gsmet gsmet requested a review from loicmathieu June 9, 2022 08:07
@evanchooly
Copy link
Member Author

Could we infer the existence of a named client by discovering uses of @MongoClientName and backfilling that client using only that name and the default values of the config? In essence, lazy init'ing the named client and letting devservices have its usual go at it

Probably, but I am thinking that it might break stuff, so I would like to avoid it.

actually, the more i think about this the less i like it. typos would be a nightmare. (even though those names should be held in constants somewhere. ;) )

@evanchooly
Copy link
Member Author

bump

@evanchooly evanchooly requested a review from geoand June 13, 2022 16:56
@geoand
Copy link
Contributor

geoand commented Jun 14, 2022

The affected test is not using dev services as the connection string has been set - mongo is being spun up by io.quarkus.test.mongodb.MongoTestResource

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM!

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jun 29, 2022

Hm... The test failure is suspicious

@quarkus-bot

This comment has been minimized.

@evanchooly
Copy link
Member Author

Hm... The test failure is suspicious

No docker on CI ...

@loicmathieu
Copy link
Contributor

@evanchooly there is no docker on Windows CI. That's why we don't use testcontainers for MongoDB tests but the testresource. But even the testresource didn't works very well on Windows due to some issue on flapdoodle side so we ends up disabling the test on Windows: @DisabledOnOs(WINDOWS).

@loicmathieu
Copy link
Contributor

@evanchooly there is no docker on Windows CI. That's why we don't use testcontainers for MongoDB tests but the testresource. But even the testresource didn't works very well on Windows due to some issue on flapdoodle side so we ends up disabling the test on Windows: @DisabledOnOs(WINDOWS).

But you didn't add new tests so I don't understand why you have this ...

@geoand
Copy link
Contributor

geoand commented Jun 30, 2022

But you didn't add new tests so I don't understand why you have this ...

Probably because:

            <plugin>
                <artifactId>maven-failsafe-plugin</artifactId>
                <configuration>
                    <skip>true</skip>
                </configuration>
            </plugin>

was removed

@evanchooly
Copy link
Member Author

fixed the windows failure by disabling it via a profile when on windows. the last CI run was all green so I squashed the commits and force pushed. we should be all good to go now.

@evanchooly evanchooly merged commit f150b14 into quarkusio:main Jun 30, 2022
@evanchooly evanchooly deleted the mdbClientDevServices branch June 30, 2022 17:02
@famod
Copy link
Member

famod commented Jul 3, 2022

It seems this PR has broken NamedMongoClientConfigTest and NamedReactiveMongoClientConfigTest on Windows. All current builds seem to fail because of this.

The failsafe-skip-profile won't work for those as both are run via surefire.

@geoand
Copy link
Contributor

geoand commented Jul 3, 2022

Yup, @evanchooly is looking into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev Services for quarkus-mongo-client don't start if @MongoClientName used
4 participants