Remove secrets usage #245

justmobilize · 2025-02-24T22:32:07Z

Remove usage of secrets and more:

change from using secrets.py to os.getenv
switch from ESPSPI_WiFiManager to WiFiManager
update pool, ssl_context setting to use adafruit_connection_manager like all the other examples
Updated some things to use f-strings

justmobilize · 2025-02-24T23:33:11Z

There are 6 PRs that use ESPSPI_WiFiManager and are the largest user of secrets in examples. This is 1 of them

dhalbert

This looks OK to me but as this is quite a vital library it would be good for @brentru and/or @tyeth to review also.

tyeth · 2025-02-26T14:47:55Z

examples/cpython/minimqtt_simpletest_cpython.py

@@ -61,7 +63,7 @@ def message(client, topic, message):

 # Set up a MiniMQTT Client
 mqtt_client = MQTT.MQTT(
-    broker=os.getenv("broker"),
+    broker="io.adafruit.com",


Maybe we stick with broker env var lookup and fall back to io.adafruit.com using the second parameter / default value, as this example isn't really IO related (just a good test server if the user already has an account). Similarly here: https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/pull/245/files#diff-afdace9507c758aee40533c5ef2f1cacbfe152f00f601e2fc07571931dfa63f8R16 which is examples/esp32spi/minimqtt_certificate_esp32spi.py line 16.

@tyeth , so do:

broker = getenv("broker", "io.adafruit.com")

in this one?

yeah, something like that. think i spotted "broker" in a few other places too (just glancing at the connect lines)

Will find all of these and make sure they are updated correctly. Will hold until I get other answers to do the work

tyeth · 2025-02-26T14:52:38Z

examples/esp32spi/minimqtt_simpletest_esp32spi.py

@@ -11,12 +11,12 @@

 import adafruit_minimqtt.adafruit_minimqtt as MQTT

-# Add settings.toml to your filesystem CIRCUITPY_WIFI_SSID and CIRCUITPY_WIFI_PASSWORD keys
-# with your WiFi credentials. Add your Adafruit IO username and key as well.
-# DO NOT share that file or commit it into Git or other source control.


We might want these warnings back (line 16), regarding not commiting/sharing settings.toml, as these examples are more intended for new users.
I'm also less of a fan of just getenv and prefer the module qualified version (os.getenv) as it's clearer what's going on for new users (the from syntax is rarely mentioned in learn guides), but personal preference maybe (and we should educate users).

So these are copied from other example. Like Requests just has:

# Get WiFi details, ensure these are setup in settings.toml ssid = os.getenv("CIRCUITPY_WIFI_SSID") password = os.getenv("CIRCUITPY_WIFI_PASSWORD")

I did my best to merge them. Happy to update these to have the warnings if wanted.

@tyeth, as far as os.getenv vs getenv, I'm good either way. The examples across the libraries are a mix. Let me know what you want here

@tyeth I'm happy with what ever you want here. Let me know

tyeth

Looks great. Only a couple of thoughts related to beginners, and using the broker address from os.getenv with a default value of io.adafruit.com wherever broker is used.
I'd quite like to run all of the examples before release, just to be sure, but won't likely get around to it until the end of the week. Equally happy for others to do so instead.

tyeth · 2025-02-26T16:32:19Z

Likewise, not too fussed, just thought worth the mention, and if memory servers the guides mostly refer to os.getenv and then link to the examples from github, but I only really recollect it in the "settings.toml / internet connect" learn guide template page that most reuse. Leave it to @brentru or @dhalbert to express a preference (or more likely just leave as is), and same with the beginner warning about not committing the settings file.

…

On Wed, 26 Feb 2025 at 16:07, Justin Myers ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/esp32spi/minimqtt_simpletest_esp32spi.py <#245 (comment)> : > @@ -11,12 +11,12 @@ import adafruit_minimqtt.adafruit_minimqtt as MQTT -# Add settings.toml to your filesystem CIRCUITPY_WIFI_SSID and CIRCUITPY_WIFI_PASSWORD keys -# with your WiFi credentials. Add your Adafruit IO username and key as well. -# DO NOT share that file or commit it into Git or other source control. @tyeth <https://github.com/tyeth>, as far as os.getenv vs getenv, I'm good either way. The examples across the libraries are a mix. Let me know what you want here — Reply to this email directly, view it on GitHub <#245 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTBZ42MA6NUUMUQLTGD46L2RXRCXAVCNFSM6AAAAABXZFATJ2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNBVGA2DSOJSGQ> . You are receiving this because you were mentioned.Message ID: <adafruit/Adafruit_CircuitPython_MiniMQTT/pull/245/review/2645049924@ github.com>

tyeth · 2025-03-01T02:55:20Z

examples/esp32spi/minimqtt_simpletest_esp32spi.py

-    port=os.getenv("port"),
-    username=os.getenv("username"),
-    password=os.getenv("password"),
+    broker="io.adafruit.com",


This example is actually meant to be optionally another server and port (just as it's simple test we might as well show the optional arg, you can use 1883 for insecure or 8883 for secure), as all it does it test callbacks and no IO specific behaviour.

tyeth · 2025-03-01T02:58:10Z

examples/esp32spi/minimqtt_pub_sub_blocking_topic_callbacks_esp32spi.py

+    broker=broker,
+    port=broker_port,
    socket_pool=pool,


Think we want to add the username and password here too, not sure how this ever worked unless it was a broker that doesn't require one (like test.mosquitto.org), but the aio_username used in topic later is now needed so we might as well offer it here too.

Suggested change

broker=broker,

port=broker_port,

socket_pool=pool,

broker=broker,

port=broker_port,

username=aio_username,

password=aio_key,

socket_pool=pool,

tyeth · 2025-03-01T02:58:29Z

examples/esp32spi/minimqtt_pub_sub_blocking_topic_callbacks_esp32spi.py

@@ -106,14 +105,14 @@ def on_message(client, topic, message):
 client.on_subscribe = subscribe
 client.on_unsubscribe = unsubscribe
 client.on_message = on_message
-client.add_topic_callback(secrets["aio_username"] + "/feeds/device.batterylevel", on_battery_msg)
+client.add_topic_callback(aio_username + "/feeds/device.batterylevel", on_battery_msg)


This (aio_username) doesn't exist, see comments earlier in file

tyeth · 2025-03-01T03:01:38Z

examples/esp32spi/minimqtt_pub_sub_blocking_topic_callbacks_esp32spi.py

+ssid = getenv("CIRCUITPY_WIFI_SSID")
+password = getenv("CIRCUITPY_WIFI_PASSWORD")
+broker = getenv("broker")
+broker_port = getenv("broker_port")



Maybe we provide defaults here too for port/broker, in addition to getting the aio_username (and I've grabbed key too)

Suggested change

ssid = getenv("CIRCUITPY_WIFI_SSID")

password = getenv("CIRCUITPY_WIFI_PASSWORD")

broker = getenv("broker")

broker_port = getenv("broker_port")

ssid = getenv("CIRCUITPY_WIFI_SSID")

password = getenv("CIRCUITPY_WIFI_PASSWORD")

aio_username = getenv("ADAFRUIT_AIO_USERNAME")

aio_key = getenv("ADAFRUIT_AIO_KEY")

broker = getenv("broker", "io.adafruit.com")

broker_port = getenv("broker_port", 1883) # Port 1883 insecure, 8883 secure

justmobilize · 2025-03-01T16:04:34Z

@tyeth I updated all the places we needed broker, fixed some f-strings, an esp32spi update from moving rssi (to match wifi.radio a while ago) and removed unsecure ports when we are passing in a ssl context.

tyeth · 2025-03-06T02:40:07Z

examples/esp32spi/minimqtt_certificate_esp32spi.py

+ssid = getenv("CIRCUITPY_WIFI_SSID")
+password = getenv("CIRCUITPY_WIFI_PASSWORD")
+broker = getenv("broker")
+username = getenv("username")
+paswword = getenv("paswword")


getenv not defined

@tyeth good catch. Fixed.

Ruff didn't catch that, so that's strange (cc @FoamyGuy)

tyeth

Just that one missing import, apart from that looks great, thanks for all the hard work!

I can't deny I wanted to suggest the "broker" environment variable be uppercase, but seeing the older fona example use it I guess either we have that as the odd one out or just leave broker alone for now.

justmobilize · 2025-03-06T05:13:40Z

Just that one missing import, apart from that looks great, thanks for all the hard work!

I can't deny I wanted to suggest the "broker" environment variable be uppercase, but seeing the older fona example use it I guess either we have that as the odd one out or just leave broker alone for now.

Happy to update, but the few places in the bundle it exists are all lowercase

tyeth · 2025-03-06T11:40:27Z

Leave it all lowercase. I'm less impressed that it passed all CI tests but with broken examples. This is my first direct experience of ruff Vs pylint, although discussed a lot with peers [seems the rule categories are insanely grouped and I dislike using numbers to disable rules as unintelligible] and can only assume that our rules are not optimally chosen yet. Attempting to use the official ruff plugin for vscode, it refuses to use ruff with this repo and in the logs shows: 2025-03-06 13:45:01.901962000 ERROR Failed to resolve settings for c:\dev\python\circuitpython\Adafruit_CircuitPython_MiniMQTT\ruff.toml: Rule `E999` was removed and cannot be selected. 2025-03-06 13:45:01.910285500 INFO Registering workspace: c:\dev\python\circuitpython\Adafruit_CircuitPython_MiniMQTT 2025-03-06 13:45:01.912912000 INFO Configuration file watcher successfully registered @FoamyGuy have you seen this behaviour before, where an undefined method/variable is not picked up by ruff?

…

On Thu, 6 Mar 2025, 05:14 Justin Myers, ***@***.***> wrote: Just that one missing import, apart from that looks great, thanks for all the hard work! I can't deny I wanted to suggest the "broker" environment variable be uppercase, but seeing the older fona example use it I guess either we have that as the odd one out or just leave broker alone for now. Happy to update, but the few places in the bundle it exists are all lowercase — Reply to this email directly, view it on GitHub <#245 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTBZ434Q7K7ROEMTEWLWHL2S7KRVAVCNFSM6AAAAABXZFATJ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBSHAZTKMBUGY> . You are receiving this because you were mentioned.Message ID: ***@***.***> [image: justmobilize]*justmobilize* left a comment (adafruit/Adafruit_CircuitPython_MiniMQTT#245) <#245 (comment)> Just that one missing import, apart from that looks great, thanks for all the hard work! I can't deny I wanted to suggest the "broker" environment variable be uppercase, but seeing the older fona example use it I guess either we have that as the odd one out or just leave broker alone for now. Happy to update, but the few places in the bundle it exists are all lowercase — Reply to this email directly, view it on GitHub <#245 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTBZ434Q7K7ROEMTEWLWHL2S7KRVAVCNFSM6AAAAABXZFATJ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBSHAZTKMBUGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Neradoc · 2025-03-06T14:46:11Z

Attempting to use the official ruff plugin for vscode, it refuses to use ruff with this repo and in the logs shows:

E999 Was removed in ruff 0.8.0 (November 2024) because it's always on now (it's syntax errors).
https://github.com/astral-sh/ruff/releases/tag/0.8.0

The cookiecutter pre-commit uses v0.3.4 (and therefore so does this repository).
I can only assume the plugin uses a more recent version.
Ruff changes very quickly it seems. Current is 0.9.9

Note the pre-commit version seemingly matches the ruff version.

Adafruit_CircuitPython_MiniMQTT/.pre-commit-config.yaml

Lines 13 to 14 in 08253c4

    
           - repo: https://github.com/astral-sh/ruff-pre-commit 
        
             rev: v0.3.4

justmobilize · 2025-03-06T15:49:39Z

@tyeth are you good with the code?

I think we can bring up the ruff issues in discord

tyeth · 2025-03-06T17:59:43Z

I haven't seen much traction in discord for some things, probably as there are too many notifications in the circuitpython-dev channel to make it a pleasant experience catching up on all discussions (and my questions being poorly framed or with no definitive answer - plus I should maybe tag people even if occasionally unwanted).

I'm happy to merge this, but I think I want a real issue raised that ruff is not working as expected possibly meaning errors are being introduced without automatic detection.
It would be helpful to have the CI show the changed files as far as pre-commit is concerned, as I can't tell which files it's run ruff against. This makes me think the issue should be raised in https://github.com/adafruit/workflows-circuitpython-libs (?) maybe with this series of commits and CI runs as the trail to follow.
Unfortunately if we merge this then we won't be able to see updated output from potential fixes in https://github.com/adafruit/workflows-circuitpython-libs although surely similar testing is possible.

Also please forgive my unfamiliarity of the circuitpython workings and processes, I normally live in arduino land and keep half an ear on circuitpython only occasionally do a bit of fixing/testing/maintenance (along with some weekend projects).

justmobilize · 2025-03-06T20:16:39Z

@tyeth I'll dig into the ruff stuff and make sure it comes up next week during in-the-weeds

tyeth · 2025-03-06T20:21:11Z

Yeah that's probably best place for it.
I might just poke the rust pre-commit version and re-enable the error to see if it will pick it up...

Plus reorder the ruff format to after fix, to comply with the newest recommendation in 0.9.9 (thanks @Neradoc )
https://github.com/astral-sh/ruff-pre-commit/blob/2c8dce6094fa2b4b668e74f694ca63ceffd38614/README.md?plain=1#L63-L65

https://github.com/astral-sh/ruff-pre-commit/blob/2c8dce6094fa2b4b668e74f694ca63ceffd38614/README.md?plain=1#L63-L65

tyeth · 2025-03-06T20:43:29Z

Okay, @justmobilize this is now the real ruff error messages (some rules were ignored without preview mode being on, and we still must be missing basic rules if I had to add F821 Undefined Name).

I've taken out the class too long one, but the rest might be actually this PR (doesn't look like it though), please take a quick look.
Only tackle what's part of your PR, and do a quick format if possible. We'll leave the rest failing for now (or I'll fix)

justmobilize · 2025-03-06T21:07:42Z

@tyeth I added ignores for the rest, which are valid for now (although could be cleaned up 1 by 1)

tyeth · 2025-03-06T21:14:56Z

@tyeth I added ignores for the rest, which are valid for now (although could be cleaned up 1 by 1)

Hero level work, thank you so much!

Making a mental note to go back and remove those after we clear up the default circuitpython library ruff rules

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 7.11.6 from 7.11.5: > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#245 from justmobilize/remove-secrets-usage

Remove secrets usage

67f971f

justmobilize marked this pull request as ready for review February 24, 2025 23:27

dhalbert requested review from brentru and tyeth February 24, 2025 23:30

dhalbert approved these changes Feb 25, 2025

View reviewed changes

tyeth reviewed Feb 26, 2025

View reviewed changes

tyeth reviewed Mar 1, 2025

View reviewed changes

PR comments

ce1a94d

justmobilize requested review from dhalbert and tyeth March 1, 2025 16:02

tyeth reviewed Mar 6, 2025

View reviewed changes

tyeth requested changes Mar 6, 2025

View reviewed changes

Missing import

8a6253d

justmobilize requested a review from tyeth March 6, 2025 05:12

tyeth approved these changes Mar 6, 2025

View reviewed changes

tyeth mentioned this pull request Mar 6, 2025

reconnect restoration #244

Merged

tyeth added 5 commits March 6, 2025 20:21

Update ruff-pre-commit, reorder format after fix, and enable error again

538c2eb

https://github.com/astral-sh/ruff-pre-commit/blob/2c8dce6094fa2b4b668e74f694ca63ceffd38614/README.md?plain=1#L63-L65

Drop rule E999 (now included for free)

ddebed1

Reorder and add rule F821 Undefined Name

054a4d0

Enable preview features in ruff

8875467

Ignore class length but keep rule active + remove error

fb2b101

tyeth self-requested a review March 6, 2025 20:42

tyeth and others added 2 commits March 6, 2025 20:46

format noqa line

a9ce58d

ruff ignores

29e12f6

tyeth approved these changes Mar 6, 2025

View reviewed changes

tyeth merged commit c413397 into adafruit:main Mar 6, 2025
1 check passed

tyeth mentioned this pull request Mar 6, 2025

Swap ruff rules, remove most noqa statements #246

Open

justmobilize deleted the remove-secrets-usage branch March 6, 2025 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove secrets usage #245

Remove secrets usage #245

justmobilize commented Feb 24, 2025 •

edited

Loading

justmobilize commented Feb 24, 2025

dhalbert left a comment

tyeth Feb 26, 2025

justmobilize Feb 26, 2025

tyeth Feb 26, 2025 •

edited

Loading

justmobilize Feb 26, 2025

tyeth Feb 26, 2025 •

edited

Loading

justmobilize Feb 26, 2025

justmobilize Feb 26, 2025

justmobilize Mar 1, 2025

tyeth left a comment •

edited

Loading

tyeth commented Feb 26, 2025 via email

tyeth Mar 1, 2025

tyeth Mar 1, 2025

tyeth Mar 1, 2025 •

edited

Loading

tyeth Mar 1, 2025

justmobilize commented Mar 1, 2025

tyeth Mar 6, 2025

justmobilize Mar 6, 2025

tyeth left a comment

justmobilize commented Mar 6, 2025

tyeth commented Mar 6, 2025 via email •

edited

Loading

Neradoc commented Mar 6, 2025 •

edited

Loading

justmobilize commented Mar 6, 2025

tyeth commented Mar 6, 2025

justmobilize commented Mar 6, 2025

tyeth commented Mar 6, 2025

tyeth commented Mar 6, 2025 •

edited

Loading

justmobilize commented Mar 6, 2025

tyeth commented Mar 6, 2025

Remove secrets usage #245

Remove secrets usage #245

Conversation

justmobilize commented Feb 24, 2025 • edited Loading

justmobilize commented Feb 24, 2025

dhalbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyeth Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyeth Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyeth left a comment • edited Loading

Choose a reason for hiding this comment

tyeth commented Feb 26, 2025 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyeth Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justmobilize commented Mar 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyeth left a comment

Choose a reason for hiding this comment

justmobilize commented Mar 6, 2025

tyeth commented Mar 6, 2025 via email • edited Loading

Neradoc commented Mar 6, 2025 • edited Loading

justmobilize commented Mar 6, 2025

tyeth commented Mar 6, 2025

justmobilize commented Mar 6, 2025

tyeth commented Mar 6, 2025

tyeth commented Mar 6, 2025 • edited Loading

justmobilize commented Mar 6, 2025

tyeth commented Mar 6, 2025

justmobilize commented Feb 24, 2025 •

edited

Loading

tyeth Feb 26, 2025 •

edited

Loading

tyeth Feb 26, 2025 •

edited

Loading

tyeth left a comment •

edited

Loading

tyeth Mar 1, 2025 •

edited

Loading

tyeth commented Mar 6, 2025 via email •

edited

Loading

Neradoc commented Mar 6, 2025 •

edited

Loading

tyeth commented Mar 6, 2025 •

edited

Loading