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

Mqtt use tls #428

Merged
merged 2 commits into from
Sep 24, 2019
Merged

Mqtt use tls #428

merged 2 commits into from
Sep 24, 2019

Conversation

fmeef
Copy link

@fmeef fmeef commented Sep 12, 2019

The MQTT_NO_TLS variable is a negative condition, where every other cmake variable is a positive condition. Change MQTT_NO_TLS to MQTT_USE_TLS to avoid confusion.

@codecov-io
Copy link

codecov-io commented Sep 12, 2019

Codecov Report

Merging #428 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #428   +/-   ##
=======================================
  Coverage   84.98%   84.98%           
=======================================
  Files          41       41           
  Lines        6392     6392           
=======================================
  Hits         5432     5432           
  Misses        960      960

@fmeef fmeef closed this Sep 12, 2019
@fmeef fmeef reopened this Sep 12, 2019
@fmeef fmeef closed this Sep 12, 2019
@fmeef
Copy link
Author

fmeef commented Sep 13, 2019

I accidentally merged in changes from master, which broke some tests in travis. It is fixed now and should work.

@fmeef fmeef reopened this Sep 13, 2019
@redboltz
Copy link
Owner

@gnu3ra , thank you for sending the PR. I have two comments.

Conclict

It seems that the PR conflict with the master. Could you rebase the PR from the up to date master? At that time, please squash commit message. You can do git push --force ....

Consistency vs Zero configuration by default

At first, mqtt_cpp has only two configuration macros. They are MQTT_USE_WS and MQTT_NO_TLS. If they aren't set, WebSocket is not supported and TLS is supported by default.
I think that the default setting is good for typical *1 usecase. MQTT is usually used in IoT domain. I guess that the message is sent via the internet so TLS support is required. If some user that has secure network (e.g.VPN), TLS isn't required. They can define MQTT_NO_TLS.

However now we have many configuration macros and all of them except MQTT_NO_TLS are positive meaning. As you mentioned, MQTT_NO_TLS is confusing.

Now we are doing breaking change for v7.0.0. I think that it is a good time to update TLS support macro. I'd like to hear @jonesmz 's opinion.

*1 I know the definition of 'typical` is difficult.

@jonesmz
Copy link
Contributor

jonesmz commented Sep 13, 2019

This line of code:

OPTION(MQTT_USE_TLS "Disable building TLS code" ON)

(Btw @gnu3ra that comment may not be accurate any longer)

sets the default setting for this CMake option to "ON", which means that, unless specified on the commandline by another flag, mqtt_cpp would be compiled with TLS support.

Note though that the current setting

OPTION(MQTT_NO_TLS "Disable building TLS code" ON)

Appears to default the library to not include TLS, because it's defaulting to "enable" "NO_TLS"


Personally, I think it would be better to default MQTT_USE_TLS = off, so that mqtt_cpp has fewer dependencies for a "default" build, but I don't have a strong opinion on it.

@redboltz
Copy link
Owner

@jonesmz , thank you for the comment. Let's replace MQTT_NO_TLS with MQTT_USE_TLS.
And the default value of MQTT_USE_TLS is OFF.

@gnu3ra , could you (force) update the PR?

@fmeef fmeef force-pushed the mqtt_use_tls branch 2 times, most recently from 5f70a1f to 9717416 Compare September 18, 2019 18:16
@fmeef
Copy link
Author

fmeef commented Sep 18, 2019

Merge conflicts are resolved, and MQTT_USE_SSL defaults to no. Does this work?

@redboltz redboltz merged commit 9a564b9 into redboltz:master Sep 24, 2019
@redboltz
Copy link
Owner

Thanks! merged.

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.

4 participants