-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Allow MQTT topic to have wildcards (# or +) #5398
base: master
Are you sure you want to change the base?
Conversation
This should fix louislam#1669
So that when wildcards for monitoring are used, the full published topic is displayed.
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.
Looks reasonable.
I have added one question below where I am a bit confused.
I have not looked into if this is a v2.0 (Bugfix of something in the beta) or v2.1 (new feature) PR
@@ -52,7 +54,7 @@ async function testMqtt(mqttSuccessMessage, mqttCheckType, receivedMessage) { | |||
} | |||
|
|||
describe("MqttMonitorType", { | |||
concurrency: true, | |||
concurrency: 4, |
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.
What is the background behind this change? How was 4 chosen? 🤔
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 almost doubled the number of tests in this describe
. For each test, a HiveMQ docker container is started. With concurrency: true
, they are all started at the same time.
My powerful laptop and servers didn't like too much starting 13 containers at exactly the same time. The test suite needed over 2 minutes, and sometimes didn't even return at all. With concurrency: 4
, the suite consistently required ~22s, which cannot get much better with a 20s timeout. There are 3 timeout tests, so concurrency: 3
would probably be fine as well. Anything more than 4 and I'd be afraid to DDOS other computers when testing.
As you wish. Since the core of this bugfix is basically just removing one |
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.
LGTM, thanks.
I am treating this as a feature
=> putting it into the v2.1 release window.
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma
Tick the checkbox if you understand [x]:
Description
Fixes #1669 : MQTT topic couldn't contain any wildcard, due to a too restrictive check.
If a message was received, it means the topic was correct, which means no further check is required.
The change is small enough that I took the liberty to not start a discussion first.
Type of change
Checklist
Screenshots (if any)
Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.