Skip to content
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 explicit checkbox when configuring MQTT over web sockets. #1875

Closed
wants to merge 1 commit into from

Conversation

tboman
Copy link

@tboman tboman commented Sep 5, 2018

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

It appears that support for MQTT over Web Sockets was intended, as some dead code already exists. This code adds a checkbox on the MQTT configuration screen following the same pattern as for "use TLS". If unchecked, current behavior stays the same. If checked, MQTT requests use ws:// instead of mqtt:// for the run-time connection to the queue.

The workaround as it stands works, but seems a little kludgy.
https://discourse.nodered.org/t/mqtt-broken-url-and-port/1275

I am holding off on creating unit tests for MQTT node, since there is no coverage at all at this time. If people agree this is a good idea, I'll create tests for the full node.

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the mailing list/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@jsf-clabot
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Tomas Boman seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 76.362% when pulling 052f1aa on tboman:topic/MQTT-over-ws into 6fa8b7f on node-red:master.

@knolleary
Copy link
Member

Hi, the node already supports websockets.
The sidebar help for the broker config node describes how to do it. So the title of this pr confused me at first until I read the details.

So the purpose of this pr is to add an explicit checkbox, rather than just rely on the broker URL?

I can't review the code until tomorrow now, but does this maintain backward compatibility for users already using websockets with the node?

It also appears your commits have been made with a different email address to your GitHub account. As we require the contributor license agreement to be signed before accepting contributions, that relies on the commits coming from identifiable users. You'll need to fix that up before we can consider merging anything.

@tboman
Copy link
Author

tboman commented Sep 5, 2018

Right, the purpose is to add an explicit checkbox. Using the scheme in the broker URL works, but I've seen a lot of people trip over it when connecting to our system that required web sockets.

@tboman tboman changed the title Add support for MQTT over web sockets. Add explicit checkbox when configuring MQTT over web sockets. Sep 5, 2018
@tboman tboman closed this Sep 7, 2018
@tboman
Copy link
Author

tboman commented Sep 7, 2018

Closed, see #1881

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants