-
Notifications
You must be signed in to change notification settings - Fork 5
Add support for connecting as named user #41
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 95.90% 96.08% +0.18%
==========================================
Files 5 5
Lines 171 179 +8
==========================================
+ Hits 164 172 +8
Misses 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@amotl would love to see this merged. I'm looking into using this library for regression tests on 5G routers, and it would be quite inconvenient for us to use a separate unauthenticated broker for testing as opposed to the testing instances we already have. |
Hi @OliverWoolland and @tiniuclx. Thanks a stack for the reminder. This certainly skipped past our attention. |
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.
Excellent. Because CI is also successful, I think it clearly demonstrates that the patch isn't harmful when running MQTT brokers without authentication. |
@@ -52,6 +62,7 @@ def setup(self): | |||
client.on_message = self.on_message | |||
if self.on_message_callback: | |||
client.on_message = self.on_message_callback | |||
client.username_pw_set(self.username, self.password) |
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.
I think it clearly demonstrates that the patch isn't harmful when running MQTT brokers without authentication.
It could make sense to not invoke this method when no credentials are defined. However, it apparently works well with Mosquitto (using guest/guest), which is the primary focus here, because pytest-mqtt brings it to the table as a testlayer broker. In this spirit, we consider it safe for merging unless/until we receive different notices.
Hi again. pytest-mqtt 0.6.0 has been released, including your improvements. Thanks again, @OliverWoolland. |
Summary
Adds optional authentication support by allowing username and password to be set via CLI arguments.
This should enable connections to brokers which disallow anonymous connections or if authentication is expected.
Defaults remain unchanged (guest/guest), so existing usage should be unaffected.
Details