-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add shared support for MongoDB Dev Services container #39787
Add shared support for MongoDB Dev Services container #39787
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Thanks for this PR. It looks very nice.
I added a few minor gripes inline as to preferring MongoDB
/mongodb
over mongo. I'm not sure it makes sense in all circumstances so feel free to push back if it doesn't.
Could you also squash all your commits when done?
Thanks!
*/ | ||
private static final String DEV_SERVICE_LABEL = "quarkus-dev-service-mongo"; | ||
|
||
private static final ContainerLocator mongoContainerLocator = new ContainerLocator(DEV_SERVICE_LABEL, MONGO_EXPOSED_PORT); |
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.
The constant should probably be named like a constant.
@@ -45,4 +45,30 @@ public class DevServicesBuildTimeConfig { | |||
@ConfigItem | |||
public Map<String, String> containerEnv; | |||
|
|||
/** | |||
* Indicates if the Mongo server managed by Quarkus Dev Services is shared. |
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.
Could you make sure you use MongoDB
instead of Mongo
in the javadoc of the configuration properties? They are published on the website.
(Note that other properties might not comply but let's not add more :))
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.
Sure, will fix it. Naming is so hard ;)
...client/deployment/src/main/java/io/quarkus/mongodb/deployment/DevServicesMongoProcessor.java
Outdated
Show resolved
Hide resolved
|
||
If you need multiple (shared) servers, you can configure the `quarkus.mongo.devservices.service-name` attribute and indicate the server name. | ||
It looks for a container with the same value, or starts a new one if none can be found. | ||
The default service name is `mongo`. |
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.
Any chance we could make it mongodb
or it is a convention and we should keep it that way?
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.
It can be changed
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.
@patrox are you sure you pushed your changes? I don't see my comments addressed and also you said you squashed everything and the commits are not.
Thanks for your feedback!
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.
@patrox are you sure you pushed your changes? I don't see my comments addressed and also you said you squashed everything and the commits are not.
Thanks for your feedback!
Yeah sorry, it looks like I messed things up during rebase. Now it should be fine :)
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.
@gsmet Now all requested changes should be in place, the commits were squashed and the branch is up-to-date with the current main
, so it looks ready!
9f7f050
to
dc6c454
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dc6c454
to
b4a7148
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dbc053e
to
4ec256d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@gsmet I've addressed all your suggestions/comments and squashed all commits after rebasing to the current |
4ec256d
to
996c099
Compare
This comment has been minimized.
This comment has been minimized.
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
bdd3edd
to
96f4627
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
96f4627
to
58778e6
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
58778e6
to
5f06435
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5f06435
to
164e21b
Compare
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.
I amended your commit to fix DEV_SERVICE_LABEL
that was still the mongo
version.
LGTM, let's merge when CI is green.
Sorry it took so much time for me to get back to it :).
Status for workflow
|
Thank you @gsmet, will wait for the CI to become 🟢 |
Status for workflow
|
@patrox thanks for your contribution, it's going to be part of 3.12. |
No description provided.