Skip to content

Add virtual host support for Rabbit Stream #37189

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add virtual host support for Rabbit Stream #37189

wants to merge 1 commit into from

Conversation

anessi
Copy link
Contributor

@anessi anessi commented Sep 5, 2023 •
edited
Loading

@pivotal-cla
Copy link

@anessi Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 5, 2023
@pivotal-cla
Copy link

@anessi Thank you for signing the Contributor License Agreement!

1 similar comment
@pivotal-cla
Copy link

@anessi Thank you for signing the Contributor License Agreement!

@mhalbritter mhalbritter mentioned this pull request Sep 5, 2023
Closed
@mhalbritter
Copy link
Contributor

mhalbritter commented Sep 5, 2023 •
edited
Loading

Looks good, thank you. The only thing I'm not sure about is the automatic fallback to spring.rabbit.virtual-host. If you got a working setup right now (virtual host for Rabbit, none for Stream), then this will break after the merge, because Stream will now use the same virtual host as Rabbit does.

@mhalbritter mhalbritter changed the title Adds a new property 'spring.rabbitmq.stream.virtual-host' which can be used to set a custom virtual host for streams Add virtual host support for Rabbit Streams Sep 5, 2023
@mhalbritter mhalbritter changed the title Add virtual host support for Rabbit Streams Add virtual host support for Rabbit Stream Sep 5, 2023
@anessi
Copy link
Contributor Author

anessi commented Sep 5, 2023 •
edited
Loading

Thanks for the feedback.

Yes, you're right that this changes the behavior slightly, but I think most people use the default virtual host / for everything. If someone considers separation by virtual host I would expect that they do it for everything, including streams. But that's opinionated or course 😄. After the change they would have to set spring.rabbit.stream.virtual-host to / to revert back to the old values.

@anessi anessi force-pushed the rabbitmq-streams-virtual-host-37123 branch from 4bfc3ac to beacd3a Compare September 5, 2023 11:11
@anessi
Copy link
Contributor Author

anessi commented Sep 5, 2023 •
edited
Loading

fixed the formatting (line length) using ./gradlew format and the tests also work now.

@anessi anessi force-pushed the rabbitmq-streams-virtual-host-37123 branch from beacd3a to 4b0f29c Compare September 5, 2023 12:03
@mhalbritter mhalbritter added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 5, 2023
@mhalbritter mhalbritter added this to the 3.2.x milestone Sep 5, 2023
@mhalbritter
Copy link
Contributor

After discussion with the team, the fallback is fine. We should add a note to the release notes how to restore the old behavior.

@mhalbritter mhalbritter self-assigned this Sep 6, 2023
mhalbritter pushed a commit that referenced this pull request Sep 6, 2023
@anessi @mhalbritter
Add virtual host support for Rabbit Stream
320dd0e 
Add a new property 'spring.rabbitmq.stream.virtual-host' which can be
used to set a custom virtual host for streams.

See gh-37189
@mhalbritter
Copy link
Contributor

Thank you very much @anessi !

@mhalbritter mhalbritter modified the milestones: 3.2.x, 3.2.0-M3 Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mhalbritter mhalbritter

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
3.2.0-M3
Development

Successfully merging this pull request may close these issues.

Add support for virtual host in RabbitProperties.Stream
4 participants
@anessi @pivotal-cla @mhalbritter @spring-projects-issues