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

WebSockets Next: honor the quarkus.http.root-path correctly #42075

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jul 23, 2024

  • i.e. do not use HttpRootPathBuildItem together with RouteBuildItem.builder()

@geoand
Copy link
Contributor

geoand commented Jul 23, 2024

So we need to figure out if we want this or not :)

@mkouba mkouba requested review from cescoffier and geoand July 23, 2024 10:45
@mkouba
Copy link
Contributor Author

mkouba commented Jul 23, 2024

So we need to figure out if we want this or not :)

Well, the opening handshake goes over HTTP so I'd say we want this 🤷

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jul 23, 2024

Clement and I were both wondering on the Zulip thread... I am still unconvinced

@cescoffier
Copy link
Member

I agree with Martin because of the "upgrade" HTTP request.

It could be debatable when you would use a direct, without-upgrade approach, but the initial HTTP request should honor the root path.

Also, I think it's more consistent from a user POV.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

The documentation should mention this.

@geoand
Copy link
Contributor

geoand commented Jul 23, 2024

The documentation should mention this.

Definitely

@mkouba
Copy link
Contributor Author

mkouba commented Jul 23, 2024

The documentation should mention this.

Done.

@mkouba mkouba added area/websockets triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jul 23, 2024

This comment has been minimized.

This comment has been minimized.

- i.e. do not use HttpRootPathBuildItem together with
RouteBuildItem.builder()
@mkouba mkouba force-pushed the ws-next-http-root branch from 4bee31a to ae33dc9 Compare July 23, 2024 14:02
Copy link

quarkus-bot bot commented Jul 23, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit ae33dc9.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

You can consult the Develocity build scans.

Copy link

quarkus-bot bot commented Jul 23, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit ae33dc9.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@cescoffier cescoffier merged commit 7df2f7b into quarkusio:main Jul 23, 2024
23 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 23, 2024
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Jul 23, 2024
Copy link

🙈 The PR is closed and the preview is expired.

@gsmet gsmet modified the milestones: 3.14 - main, 3.13.2 Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants