Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Reapply "Switch Vert.x HTTP to @ConfigMapping" #45274

Closed
wants to merge 1 commit into from

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Dec 24, 2024

This reverts commit 5f90f04 and reapply the Switch Vert.x HTTP to @ConfigMapping change.

This was originally written by Roberto, it conflicted all around with all the new changes that got in so it was a lot of fun rebasing :)

@radcortez I don't know if you remember but this change broke quite a lot of extensions out there. I wonder if we should apply the same trick and have the interface with a new name and keep the old config as deprecated with maybe only the ports? I think if we keep the compatibility for the ports, we fix 99% of the breaking change. We might need to add a few more such as rootPath but I would rather be conservative and get the feedback from Ecosystem CI first.

Note that I will have a look at the consequences of this change this afternoon so that we hopefully don't have to revert later.

Fixes #44115

This reverts commit 5f90f04.

This was originally written by Roberto, it conflicted all around with
all the new changes that got in so it was a lot of fun rebasing :)

Co-authored-by: Roberto Cortez <radcortez@yahoo.com>
Copy link

🎊 PR Preview 4f943d5 has been successfully built and deployed to https://quarkus-pr-main-45274-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@radcortez
Copy link
Member

I was already working on it but didn't have a PR yet:
radcortez@5159607

Mainly because I was working out some SR Config improvements to compensate some of the extra work:
smallrye/smallrye-config#1273

I have a couple of other ideas for improving things.

@radcortez I don't know if you remember but this change broke quite a lot of extensions out there. I wonder if we should apply the same trick and have the interface with a new name and keep the old config as deprecated with maybe only the ports? I think if we keep the compatibility for the ports, we fix 99% of the breaking change. We might need to add a few more such as rootPath but I would rather be conservative and get the feedback from Ecosystem CI first.

Yes, we need to keep the old configs for ports and rootPath to avoid major breakage.

@gsmet
Copy link
Member Author

gsmet commented Jan 7, 2025

@radcortez so do you prefer I close this one? Sorry, I thought it would be a good holiday project when everything was quiet :).

@radcortez
Copy link
Member

@radcortez so do you prefer I close this one? Sorry, I thought it would be a good holiday project when everything was quiet :).

No worries. We can use this one. Let's nail some of the performance improvements first.

@gsmet
Copy link
Member Author

gsmet commented Jan 22, 2025

We can close this as we have another PR going on: #45769

@gsmet gsmet closed this Jan 22, 2025
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Jan 22, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Quarkus Vert.x HTTP extension config classes to @ConfigMapping
2 participants