[PR from CLI tool] Allow connectivity configs in MQTT demo to be CLI overridden #1124
Conversation
Codecov Report
@@ Coverage Diff @@
## development #1124 +/- ##
===============================================
+ Coverage 96.54% 98.47% +1.93%
===============================================
Files 9 4 -5
Lines 5643 1313 -4330
Branches 641 391 -250
===============================================
- Hits 5448 1293 -4155
+ Misses 9 0 -9
+ Partials 186 20 -166
Continue to review full report at Codecov.
|
| @@ -56,26 +56,30 @@ | |||
| * the instructions in https://mosquitto.org/ for running a Mosquitto broker | |||
| * locally. | |||
| */ | |||
| #define BROKER_ENDPOINT "test.mosquitto.org" | |||
| #ifndef BROKER_ENDPOINT | |||
There was a problem hiding this comment.
The philosophy of this file demo_config.h already is that applications can edit it.
In mqtt_demo_basic_tls.c we should have:
#ifndef BROKER_ENDPOINT
#error "Please define an MQTT broker endpoint, BROKER_ENDPOINT in demo_config.h."
#endif
There was a problem hiding this comment.
I agree, that our demo source files should have build-time failures for no definitions of these macros. Have added them in source files.
There was a problem hiding this comment.
So how would this build-time failures for no definitions of these macros work for our demo sources? Wont this file always define BROKER_ENDPOINT ? So by the time we get to our demo source files, wont the check for ifndef BROKER_ENDPOINT always say that BROKER_ENDPOINT is defined?
There was a problem hiding this comment.
It is to guard against accidental removals of the BROKER_ENDPOINT by the customer. It provides a helpful message to remind them that the BROKER_ENDPOINT macro is required for the demo to run instead of giving them regular gcc build failures (at the source code points that use the macro)
There was a problem hiding this comment.
This is still different from what the README says about Mutual Auth Demos.
| @@ -61,8 +61,11 @@ | |||
| * These configuration settings are required to run the basic TLS demo. | |||
| * Throw compilation error if the below configs are not defined. | |||
| */ | |||
| #ifndef BROKER_ENDPOINT | |||
| #error "Please define an MQTT broker endpoint, BROKER_ENDPOINT, in demo_config.h." | |||
There was a problem hiding this comment.
Will this error ever trigger? Because we defined this macro in the config if it wasn't defined already
There was a problem hiding this comment.
If the customer happens to remove it from the config file, then this failure would remind them instead.
| @@ -53,7 +53,9 @@ | |||
| * This demo uses the Mosquitto test server. This is a public MQTT server; do not | |||
| * publish anything sensitive to this server. | |||
| */ | |||
| #define BROKER_ENDPOINT "test.mosquitto.org" | |||
| #ifndef BROKER_ENDPOINT | |||
| #define BROKER_ENDPOINT "test.mosquitto.org" | |||
There was a problem hiding this comment.
The cautionary language on lines 53-55 is not sufficient here. From the test.mosquitto.org documentation, the test.mosquitto.org broker "...will often be running unreleased or experimental code and may not be as stable as you might hope.". Additional cautionary language on their website asks "...please do not abuse or rely upon it for anything of importance.".
I would recommend removing references to test.mosquitto.org from this (and any other) MQTT demos. If someone wishes to use it, they of course may do so. However the best way to test this by running a broker locally, and we can easily provide instructions for setting up mosquitto or another broker to run locally.
There was a problem hiding this comment.
Such directions are available in the readme too.
There was a problem hiding this comment.
It's a good suggestion to not suggest test.mosquitto.org in our demos. I had added instructions on the REAMDE for a way to setup the Mosquitto broker locally: https://github.com/aws/aws-iot-device-sdk-embedded-C/tree/development#installing-mosquitto-to-run-mqtt-demos-locally
We could update the comments on our demo_config.h to point to these instructions
…overridden (aws#1124) * Add #ifndefs around config macros in demos to allow them to be overridden * Add #error statements for BROKER_ENDPOINT in demo source files
…overridden (aws#1124) * Add #ifndefs around config macros in demos to allow them to be overridden * Add #error statements for BROKER_ENDPOINT in demo source files
…overridden (aws#1124) * Add #ifndefs around config macros in demos to allow them to be overridden * Add #error statements for BROKER_ENDPOINT in demo source files
…overridden (aws#1124) * Add #ifndefs around config macros in demos to allow them to be overridden * Add #error statements for BROKER_ENDPOINT in demo source files
…overridden (aws#1124) * Add #ifndefs around config macros in demos to allow them to be overridden * Add #error statements for BROKER_ENDPOINT in demo source files
Allow
BROKER_ENDPOINTandROOT_CA_CERT_PATH(only for basic TLS demo) to be specified through build command for CI.By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.