Migrate configuration handling to the confuse lib, take 2 #364

jinnatar · 2023-02-10T20:23:21Z

This enables:

Conservative config defaults in src/default_config.yaml, which leads to smaller and leaner custom configs.
Config validation with confuse templates or at-use get methods with type checking.
Easier future mutation of config format as defaults can carry over sane defaults.

This change touches all integrations and has minor potential for subtle breakage.

The following notifiers have been thoroughly tested:

Telegram
Pushover
IFTTT
MQTT
SMTP
Script
Discord

Notifiers not tested:

Slack

Notifiers with known issues:

Grafana, WIP

Notifiers I cannot test myself:

Pushcut

Groundwork for fixing #361

I've tested and verified the following to function: - Telegram - Pushover

This ensures the backwards compatibility is still carried forward of enabling misconfigured handlers.

This prevents it getting interpreted as a valid notifier and causing a warning.

Now only the showcases are run where other tokens are also present

Also, switch SMTP testing to verify SMTP_HOST, not SMTP_USERNAME as username is not a mandatory param for smtp to work, but a host is.

Also amend SMTP testing to allow setting SMTP auth

martomi

Excellent work, thanks!

martomi · 2023-02-12T12:26:51Z

INTEGRATIONS.md

+SMTP_ENABLE_AUTH=true \
+SMTP_USERNAME=username \
+SMTP_PASSWORD=password \
+python3 -m unittest tests.notifier.test_smtp_notifier


Thanks for fixing these!

martomi · 2023-02-12T12:28:25Z

README.md

@@ -141,8 +141,6 @@ git pull
 ./install.sh
 ```

-> Important: Automated migration of config is not supported. Please check that your `config.yaml` has all new fields introduced in `config-example.yaml` and add anything missing. If correctly migrated, you shouldn't get any ERROR logs.


martomi · 2023-02-12T12:31:01Z

config-example.yaml

@@ -29,7 +31,7 @@ keep_alive_monitor:
 # Enable this and you'll receive a daily summary notification
 # on your farm performance at the specified time of the day.
 daily_stats:
-  enable: true
+  enable: true # default: false


This is excellent comment! In a future iteration (e.g. when we bump the major version to 1.0) we can switch to more optimistic defaults but as discussed in the previous PR, now we’re keeping the conservative defaults from the code.

martomi · 2023-02-12T12:37:43Z

src/chia_log/log_consumer.py

+    "remote_host": str,
+    "remote_user": str,
+    "remote_port": int,
+}


martomi · 2023-02-12T13:01:37Z

@Artanicus sorry, I might have merged while you’re still in WIP (just read the updated PR description). Feel free to finish the rest in a separate PR, as this one was already looking good and was quite large so I wanted to have it in master as a base for other changes :-)

jinnatar · 2023-02-12T14:51:07Z

No worries, all in all it's fairly solid. Slack I haven't tested yet (slightly more complex integration) but will, Grafana I know fails because it violates a few notifier conventions (have a WIP fix I'll PR separately since it's a bit opinionated), and Pushcut I can't test since they support such a limited set of platforms.

jinnatar added 21 commits February 10, 2023 21:26

WIP: Transform log parsing and handling to use the confuse lib

d121e0a

Default log level to INFO, include stern warning with defaults

8050def

Implement the confuse library handling in notifiers

0c9f7a0

I've tested and verified the following to function: - Telegram - Pushover

Expect ConfigViews correctly in typing

2d20311

Simplify wallet mojo testing

39d395b

Amend documentation for new config handling.

edb7506

Explicitly handle daily_stats average times as floats

42676a8

Make non_skipped_signage_points checking typing explicit

198e293

Type signage_point_stats init values explicitly

527eefe

Fix silly confuse import

d811a0e

More explicit typing and minor cleanup

6208892

Validate log consumer config via confuse templates

2ea6b24

Add more type safety to notifier parsing

96ebce9

Fix handler enable checking

8d51de6

This ensures the backwards compatibility is still carried forward of enabling misconfigured handlers.

Also disable confuse.exceptions type checking since there are no stubs

83db911

Fix template formatting with black

95204a3

Set config defaults to the conservative values that were in code

a73aaea

Move notifier_defaults config block to top level

5daca4f

This prevents it getting interpreted as a valid notifier and causing a warning.

Add comments to config-example.yaml where the default differs

a9e8baf

Handle MQTT notifier type checking via confuse

c88cdb8

Improve testability of tests that use SHOWCASE_NOTIFICATIONS

4717d3d

Now only the showcases are run where other tokens are also present

jinnatar mentioned this pull request Feb 11, 2023

Fall back to SMTP without auth if server doesn't support it #365

Merged

jinnatar added 2 commits February 12, 2023 13:54

Dedupe test env variables of mqtt & smtp with prefixes

ea1e0e3

Also, switch SMTP testing to verify SMTP_HOST, not SMTP_USERNAME as username is not a mandatory param for smtp to work, but a host is.

Validate SMTP notifier credentials with template

d6be587

Also amend SMTP testing to allow setting SMTP auth

martomi approved these changes Feb 12, 2023

View reviewed changes

martomi merged commit 228b23d into martomi:main Feb 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate configuration handling to the confuse lib, take 2 #364

Migrate configuration handling to the confuse lib, take 2 #364

jinnatar commented Feb 10, 2023 •

edited

Loading

martomi left a comment

martomi Feb 12, 2023

martomi Feb 12, 2023

martomi Feb 12, 2023

martomi Feb 12, 2023

martomi commented Feb 12, 2023

jinnatar commented Feb 12, 2023

Migrate configuration handling to the confuse lib, take 2 #364

Migrate configuration handling to the confuse lib, take 2 #364

Conversation

jinnatar commented Feb 10, 2023 • edited Loading

martomi left a comment

Choose a reason for hiding this comment

martomi Feb 12, 2023

Choose a reason for hiding this comment

martomi Feb 12, 2023

Choose a reason for hiding this comment

martomi Feb 12, 2023

Choose a reason for hiding this comment

martomi Feb 12, 2023

Choose a reason for hiding this comment

martomi commented Feb 12, 2023

jinnatar commented Feb 12, 2023

jinnatar commented Feb 10, 2023 •

edited

Loading