search

quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.87k stars 2.71k forks source link

Migrate Quarkus Vert.x HTTP extension config classes to @ConfigMapping #44115

Open michalvavrik opened 1 month ago

michalvavrik commented 1 month ago

Description

I have mentioned many extensions are migrating to @ConfigMapping. I am waiting for a moment when the Vert.x HTTP extension will be migrated because lately we talked once again with @sberyozkin about fluent API to setup Security https://github.com/quarkusio/quarkus/issues/16728 and - this is just my opinion - interfaces would help because they establish contract. If someone changes field or adds a new field of a config class, we might forget to update builders. But if the @ConfigMapping interface changed, builders we could provide will simply fail to compile.

Now, I know that @radcortez already migrated this extension https://github.com/quarkusio/quarkus/pull/35246 and then reverted it https://github.com/quarkusio/quarkus/pull/35756, but since then, I have seen so many migrations, lately in core module https://github.com/quarkusio/quarkus/issues/42114 that I don't understand why is this extension so special. We should have at least plan or tracker and this is why I am opening this issue.

Implementation ideas

I am happy to help, but I think Roberto has it covered. For users, we could provide update recipe, for extension writers, I think they can handle...

quarkus-bot[bot] commented 1 month ago

/cc @radcortez (config)

radcortez commented 1 month ago

Now, I know that @radcortez already migrated this extension #35246 and then reverted it #35756, but since then, I have seen so many migrations, lately in core module #42114 that I don't understand why is this extension so special. We should have at least plan or tracker and this is why I am opening this issue.

This is definitely not forgotten; I have a local branch with that work. We had to revert it at the time due to performance issues, but those problems were already fixed, so it should be safe to move.

What is holding this for now is that many extensions use the Vert.x http configuration directly to retrieve the port and a few other things so that it will affect many extensions. Extensions should not be using this, and we were discussing better alternatives for it:

So ideally, we wanted to introduce a better API to retrieve the port, instead of relying on the configuration, so extensions would only need to move once and not twice (from Root -> to new API, instead of from Root -> Mapping -> new API).

But I guess this is taking too long, so we might want to go the not-so-optimal route.

michalvavrik commented 1 month ago

I have subscribed myself to all three PRs so that I get notified. Thanks for the links. I'll leave this issue opened so that there is a tracker for this migration (so that I get notified as well). Thank you

michalvavrik commented 1 month ago

To be fair, I have loads of other issues I can work on, we are in no hurry, so please migrate at your own pace.