Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements RabbitMQ permission handling by introducing a "check before create" pattern for exchanges and queues. When a client lacks permissions to create resources, the code first attempts to verify the resource exists (using passive=True), and only attempts creation if it doesn't exist (404 error). This allows limited-permission clients to work with existing resources while still enabling resource creation when permitted.
Key Changes:
- Added passive checking pattern for exchange and queue declarations in producer and consumer
- Changed
queue_suffixparameter default fromFalseto""(empty string) for better type consistency - Added comprehensive permission tests to validate behavior with restricted users
- Updated RabbitMQ configuration to support permission-based testing
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| varys/producer.py | Implements passive checking for exchanges/queues; adds exception handling for permission-based resource management |
| varys/consumer.py | Implements passive checking for exchanges/queues; adds exception handling for permission-based resource management |
| varys/controller.py | Changes queue_suffix default from False to "" for type consistency |
| tests/test_varys.py | Adds comprehensive test suite for permission scenarios including extant/non-extant resources |
| .rabbitmq/definitions.json | Defines RabbitMQ users and permission patterns for testing |
| .rabbitmq/rabbitmq.conf | Configures RabbitMQ to load permission definitions |
| .github/workflows/pytest.yml | Updates RabbitMQ Docker version and increases test timeout to accommodate permission tests |
| setup.cfg | Version bump to 1.2.0 |
| .gitignore | Adds varys_client.egg-info directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
varys/producer.py:136
- After catching a 404 error for the queue, the code re-declares the exchange with
passive=True. This is unnecessary since the exchange already exists (it was successfully declared or verified earlier in lines 98-117). The exchange declaration here should either be removed, or if it's needed for safety, should not usepassive=Truesince we're in a creation flow.
self._channel.queue_declare(queue=self._queue, durable=True)
self._channel.queue_bind(
queue=self._queue,
exchange=self._exchange,
routing_key=self._routing_key,
varys/consumer.py:112
- After catching a 404 error for the queue, the code re-declares the exchange with
passive=True. This is unnecessary since the exchange already exists (it was successfully declared or verified earlier in lines 75-94). The exchange declaration here should either be removed, or if it's needed for safety, should not usepassive=Truesince we're in a creation flow.
self._channel.queue_declare(queue=self._queue, durable=True)
self._channel.queue_bind(
queue=self._queue,
exchange=self._exchange,
routing_key=self._routing_key,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.