-
-
Notifications
You must be signed in to change notification settings - Fork 969
Allow dots in Azure Service Bus queue names #2221
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2221 +/- ##
=======================================
Coverage 81.54% 81.54%
=======================================
Files 77 77
Lines 9517 9517
Branches 1154 1154
=======================================
Hits 7761 7761
Misses 1564 1564
Partials 192 192 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this going to be a breaking change?
Depends on how a "breaking change" is defined I think, but if a breaking change is defined as a change to the public interface, then I don't think so:
So it would only impact users who:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting change. Please rename the PR to a meaningful title.
Also, take note we’re in the middle of a (long) release cycle so this change cannot be merged at least until we finish the release.
Thank you for your patience.
I renamed the PR. No problem if this takes a while to get merged in, we have a workaround on our end |
Cool. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the Azure Service Bus transport so that queue names can retain dots instead of converting them to dashes.
- Removed the dot-to-dash mapping in the character replacement table.
- Adjusted unit tests and comments to assert that dots are allowed in entity names.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
t/unit/transport/test_azureservicebus.py | Updated comments and assertions to verify dots are preserved |
kombu/transport/azureservicebus.py | Removed ord('.') → ord('-') mapping in CHARS_REPLACE_TABLE |
Comments suppressed due to low confidence (2)
kombu/transport/azureservicebus.py:85
- Update this comment to reflect that dots are now allowed. For example: 'dots and dashes allowed; all other punctuation replaced by underscore.'
# dots are replaced by dash, all other punctuation replaced by underscore.
t/unit/transport/test_azureservicebus.py:369
- Add a test case to verify that disallowed punctuation (e.g., '!') is replaced by underscores, for example:
assert channel.entity_name('test?celery') == 'test_celery'
.
assert channel.entity_name('test_celery') == 'test_celery'
In the Azure Service Bus transport, dots are currently replaced by dashes. This should not be the case, since https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftservicebus allows dots in queue names (it's possible that the conventions have changed)
I realise dots inside a queue name are probably not best practice, but in our case the queue names are determined by a third party and out of our control.