-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Convert mongodb-client to use @ConfigMapping #46070
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ccompile error |
a043ed8
to
9d261c9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongodbConfig.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview d656dc3 has been successfully built and deployed to https://quarkus-pr-main-46070-preview.surge.sh/version/main/guides/
|
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.
Looks good overall but I noticed a small issue.
Could you adjust and squash the commits?
Thanks!
@WithConverter(TrimmedStringConverter.class) | ||
Optional<String> credentialsProvider(); |
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.
We want Optional<@WithConverter(TrimmedStringConverter.class) String>
and same for the other occurrences.
cd197a9
to
5bff774
Compare
2715f2b
to
a08b744
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a08b744
to
84cfa31
Compare
Sorry we collided :). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongodbConfig.java
Outdated
Show resolved
Hide resolved
extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongodbConfig.java
Outdated
Show resolved
Hide resolved
I will finalize things. |
5aa470f
to
a620f6a
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.
It should be all good now and I rebased to get the CI fix.
Status for workflow
|
Status for workflow
|
I am the maintainer of the quarkus-mongock extension. I just noticed that this change breaks the build of the extension : https://github.com/quarkiverse/quarkus-mongock/actions/runs/13150659666/job/36704570744 I wonder how can I take this change into account in a backward-compatible way? Thanks, |
@tms0 hey. We will communicate much larger about this soon. Roberto sent an email yesterday on quarkus-dev and I was going to have a look at the Ecosystem CI failures today. Unfortunately, this is a bit of a big bang change. We know it's not ideal but we really need to move to this new infrastructure. We could have the option of restoring just enough old config (I think you just need the database name) for a couple of months but we will drop the old infrastructure at some point anyway so I'm not sure it's worth it. I checked if there could be other alternatives but I don't see anything that could provide you with the database name. I don't know if your extension evolves a lot, do you think you could move Thanks for reaching out :). |
@tms0 that being said, you could use an approach similar to what @gastaldi did here: https://github.com/quarkiverse/quarkus-ironjacamar/pull/158/files#diff-546c20e149ef134f4ad79befb3f960d9ef06f1945aea77fdfd92fdd9cf845880R128-R130 . Ideally you would get back to using proper config mapping in the future but it could ease the transition. |
Part of #45446