-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add scheme support #12
Conversation
This allows a user to specify the scheme to use with the MQTT broker in `config.yml`. For example `mqtts`.
@@ -470,7 +471,7 @@ func loadConfig() { | |||
debug("Disconnected from MQTT (monitoring)") | |||
} | |||
opts := mqtt.NewClientOptions() | |||
opts.AddBroker(fmt.Sprintf("%s://%s:%d", "tcp", getConfig().Monitor.MQTT.Server, getConfig().Monitor.MQTT.Port)) | |||
opts.AddBroker(fmt.Sprintf("%s://%s:%d", "tcp", getConfig().Monitor.MQTT.Scheme, getConfig().Monitor.MQTT.Server, getConfig().Monitor.MQTT.Port)) |
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.
You seem to be having a syntax error here (3 variables in the format string, 4 passed as argument - the "tcp" argument seems to be unneccessary). Nevertheless, based on the documentation, the proper format seems to be the following: ssl://host:port
for mqtts (or tcp://host:port
in case of mqtt), are you sure it works with mqtt/mqtts://?
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.
Based on the code it does accept multiple variations, however in order to avoid introducing a new config variable, I would simply use the following:
opts.AddBroker(fmt.Sprintf("%s:%d", getConfig().Monitor.MQTT.Server, getConfig().Monitor.MQTT.Port))
This way you can specify the scheme in the server address (e.g. mqtts://host
or tcp://host
or mqtt://host
etc.) and if you don't, standard mqtt (tcp) will be assumed by the mqtt client library. Care to test this?
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.
Yeah I missed the tcp
bit when I forked and did the PR, the line I actually used in the local code I'm using didn't have it (which is why it was working for me I expect).
I've just tested your version, which makes more sense, and it works fine specifying the scheme in the server address:
opts.AddBroker(fmt.Sprintf("%s:%d", getConfig().Monitor.MQTT.Server, getConfig().Monitor.MQTT.Port))
As an aside, ssl://
does also work, I just wasn't aware you could use ssl://
instead of mqtts://
. So I'll leave it up to you (might be worth adding something in the documentation about schemes?).
I'll close this PR, as @a-bali change works well. |
As per issue #11 I had a bit more of a dig around and was able to add support for specifying the scheme (
mqtt
ormqtts
).I've built it locally and I'm running it fine in my Home Assistant setup. It's simple enough (so doesn't check if the value is
mqtt
ormqtts
), but does the job for me great.Hopefully if you want to add support for this you can use this PR as a starting point (not that it was in anyway complicated to add support for!). I'll continue to use my branch locally, but would be nice if it could be merged for anybody else that may come across the issue I had!
Thanks 👍