Conversation
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds initial JMS queue support to the RabbitMQ AMQP 1.0 Go client, aligned with the Tanzu RabbitMQ feature set referenced in issue #90.
Changes:
- Introduces a new
Jmsqueue type andJmsQueueSpecification, plus unit tests and a new JMS example. - Adds a queue-spec validation hook in
DeclareQueueand extends feature detection to include Tanzu/version flags. - Updates documentation and changelog to reference Tanzu RabbitMQ requirements and the new example.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds a Tanzu RabbitMQ note and link for Tanzu-only features (incl. JMS). |
| pkg/rabbitmqamqp/features_available.go | Extends feature flags to include 4.3+ and Tanzu detection. |
| pkg/rabbitmqamqp/entities.go | Adds Jms queue type, JmsQueueSpecification, and a new validate method on queue specs. |
| pkg/rabbitmqamqp/entities_test.go | Adds unit tests for JmsQueueSpecification arguments and fixed flags. |
| pkg/rabbitmqamqp/amqp_management.go | Calls queue-spec validation before declaring a queue; wires features into management. |
| pkg/rabbitmqamqp/amqp_management_test.go | Updates management construction to pass feature state. |
| pkg/rabbitmqamqp/amqp_connection.go | Shares a single featuresAvailable instance between connection and management. |
| pkg/rabbitmqamqp/amqp_connection_recovery.go | Adds recovery record → JmsQueueSpecification mapping. |
| pkg/rabbitmqamqp/amqp_connection_recovery_test.go | Leaves JMS recovery table entries commented out. |
| docs/examples/web_sockets/README.md | Updates Tanzu RabbitMQ link. |
| docs/examples/README.md | Adds JMS queue example entry. |
| docs/examples/jms_queue/main.go | New example showing how to declare and use a JMS queue. |
| CHANGELOG.md | Notes the new JMS queue type/spec and example. |
| AGENTS.md | Documents the new Jms queue type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -31,6 +32,7 @@ type IQueueSpecification interface { | |||
| isExclusive() bool | |||
| queueType() TQueueType | |||
| buildArguments() map[string]any | |||
There was a problem hiding this comment.
Adding validate(*featuresAvailable) error to the exported IQueueSpecification interface is a breaking API change for any downstream users that implement custom queue specifications. Consider making validation optional (e.g., a separate optional interface checked via type assertion inside DeclareQueue) or providing a backwards-compatible adapter, instead of extending the public interface.
| buildArguments() map[string]any | |
| buildArguments() map[string]any | |
| } | |
| // iQueueSpecificationWithValidation is an internal, optional extension of IQueueSpecification | |
| // for specifications that support validating themselves against available features. | |
| // Code that needs validation should use a type assertion against this interface | |
| // instead of requiring all IQueueSpecification implementations to define validate. | |
| type iQueueSpecificationWithValidation interface { | |
| IQueueSpecification |
| type featuresAvailable struct { | ||
| is4OrMore bool | ||
| is41OrMore bool | ||
| is42rMore bool | ||
| is43rMore bool | ||
| isRabbitMQ bool | ||
| isTanzu bool | ||
| } |
There was a problem hiding this comment.
The newly added is43rMore / isTanzu flags are computed but not referenced anywhere else in the codebase. If they’re intended to gate JMS queue declarations, wire them into the new queue-spec validation (or remove them until they’re used) to avoid dead state and future confusion.
| //Entry("JMS queue", Jms, false, false, map[string]any{}), | ||
| Entry("Quorum queue with arguments", Quorum, false, false, map[string]any{"x-max-length-bytes": 1000}), | ||
| Entry("Classic queue with arguments", Classic, true, true, map[string]any{"x-max-length-bytes": 1000}), | ||
| Entry("Stream queue with arguments", Stream, false, false, map[string]any{"x-max-length-bytes": 1000}), | ||
| //Entry("JMS queue with arguments", Jms, false, false, map[string]any{"x-max-length-bytes": 1000}), |
There was a problem hiding this comment.
The JMS queue entries in this table test are commented out, which leaves the new Jms recovery-path (queueRecoveryRecord.toIQueueSpecification) untested. If the test is expected to pass for JMS as it does for other queue types, please re-enable these entries (or replace with an explicit Skip/conditional) so regressions are caught.
| //Entry("JMS queue", Jms, false, false, map[string]any{}), | |
| Entry("Quorum queue with arguments", Quorum, false, false, map[string]any{"x-max-length-bytes": 1000}), | |
| Entry("Classic queue with arguments", Classic, true, true, map[string]any{"x-max-length-bytes": 1000}), | |
| Entry("Stream queue with arguments", Stream, false, false, map[string]any{"x-max-length-bytes": 1000}), | |
| //Entry("JMS queue with arguments", Jms, false, false, map[string]any{"x-max-length-bytes": 1000}), | |
| Entry("JMS queue", Jms, false, false, map[string]any{}), | |
| Entry("Quorum queue with arguments", Quorum, false, false, map[string]any{"x-max-length-bytes": 1000}), | |
| Entry("Classic queue with arguments", Classic, true, true, map[string]any{"x-max-length-bytes": 1000}), | |
| Entry("Stream queue with arguments", Stream, false, false, map[string]any{"x-max-length-bytes": 1000}), | |
| Entry("JMS queue with arguments", Jms, false, false, map[string]any{"x-max-length-bytes": 1000}), |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
part of #90
This PR adds JMS queue support to the RabbitMQ AMQP 1.0 Go client, aligned with the Tanzu RabbitMQ feature set referenced in issue #90.
Changes:
Jmsqueue type andJmsQueueSpecification, plus unit tests and a new JMS example.DeclareQueueand extends feature detection to include Tanzu/version flags.