Synthead support options from cli and config file #41

pliablepixels · 2018-03-31T17:38:41Z

No description provided.

…ction.

…support-options-from-cli-and-config-file

…rd compat

pliablepixels · 2018-03-31T17:39:02Z

Modifications to #40

pliablepixels · 2018-03-31T17:42:34Z

Some notes to your PR @synthead
Not a perl expert, so here is what I did

The API key needs to fall back to my API key for FCM to work (most people who use push won't change it)
I'd prefer tokens.txt to be in /etc/private as doing it elsewhere will break old paths
So the $config->val(..,..,'default') -> default was just not working - was it working for you?
Ok with no cert defaults, but would prefer a default value in the ini file

(My entire goal is people migrating from old versions to this new version only has to place zmeventnotification.ini in /etc/ and it should just start working)

synthead

Looking good! Left a few items for you to take a peek at, notably:

Use of || as opposed to $config->val( ... ) defaults.
Users deliberately setting blank values in the INI file.
Small cleanup items.
Semantic and grammar things here and there.

Please let me know what you think, and don't hesitate to get a conversation started if you are looking to share ideas!

synthead · 2018-03-31T23:11:27Z

README.md


 ```
 sudo zmdc.pl start zmeventnotification.pl
 ```

 Make sure you look at the syslogs to make sure its started properly

+## Understanding zmeventnotification configuration


This is awesome! 💯

synthead · 2018-03-31T23:12:42Z

zmeventnotification.pl

 if (!try_use ("Net::WebSocket::Server")) {Fatal ("Net::WebSocket::Server missing");exit (-1);}
 if (!try_use ("IO::Socket::SSL")) {Fatal ("IO::Socket::SSL  missing");exit (-1);}
+if (!try_use ("Config::IniFiles")) {Fatal ("Config::Inifiles  missing");exit (-1);}


synthead · 2018-03-31T23:14:34Z

zmeventnotification.pl

+
+  --enable-fcm                        Use FCM for messaging (default: true).
+  --no-enable-fcm                     Don't use FCM for messaging (default: false).
+  --fcm-api-key=KEY                   API key for FCM.


Since the internal FCM key is the default, I would add a description like:

API key for FCM (default: general purpose key)

synthead · 2018-03-31T23:18:14Z

zmeventnotification.pl

+
+  --help                              Print this page.
+
+  --config=FILE                       Read options from configuration file (default: /etc/zmeventnotification.ini).


I think the Using --config is preferred bit is somewhat subjective as there's no "right" way to declare options. Maybe we should update the description to say something like:

--config=FILE Read options from configuration file (default: /etc/zmeventnotification.ini). Any CLI options issued will override the settings in this file.

Speaking of being subjective, I think this fits a little better with the formatting, too 👍

The issue here is most people won't run it from command line - they will daemonize it via zmdc and that requires you pass the same parameters inside the modifications to zmdc and zmpkg, which people will likely forget to do. I've moved the recommendation to README instead

synthead · 2018-03-31T23:19:47Z

zmeventnotification.pl

+  }
+} else {
+  $config = Config::IniFiles->new;
+  Info ("No config file found, using inbuilt defaults");


synthead · 2018-03-31T23:33:27Z

README.md

+Auth timeout .................. 20
+
+Use FCM ....................... true
+FCM API key ................... <sequence of characters>


Just a note: there's a comment later on about printing the key here. If you decide to change it, update this text.

synthead · 2018-03-31T23:34:22Z

README.md

@@ -259,7 +313,7 @@ Oct 20 10:28:56 homeserver zmeventnotification[27789]: INF [Pushproxy push messa
 ## For Developers writing their own consumers

 ### How do I talk to it?
-*  ``{"JSON":"everywhere"}``
+*  `{"JSON":"everywhere"}`


synthead · 2018-03-31T23:36:50Z

zmeventnotification.ini

+enable = 1
+# Custom FCM API key. Leave this empty if you are not using
+# your own API key (most people will leave this empty)
+api_key =


I thiiiiiink this will deliberately set an undefined value. I thiiiink. Double-check that. If it does, then this is the condoned behavior of Config::IniFiles and we should abide by it by commenting this out.

This is subjective, but I think we should allow very explicit settings by the user. If they set it to nothing, allow them to use nothing. Doing something else feels like erroneous behavior since the user deliberately set a key instead of commenting this out.

synthead · 2018-03-31T23:37:21Z

zmeventnotification.ini

+# your own API key (most people will leave this empty)
+api_key =
+# Auth token store location (default: /etc/private/tokens.txt).
+token_file = 


Ditto here. If this is set to nothing, let it be nothing and don't take the default.

synthead · 2018-03-31T23:38:01Z

zmeventnotification.pl

-my $notId = 1;
-
+#default key. Please don't change this
+use constant NINJA_API_KEY => "AAAApYcZ0mA:APA91bG71SfBuYIaWHJorjmBQB3cAN7OMT7bAxKuV3ByJ4JiIGumG6cQw0Bo6_fHGaWoo4Bl-SlCdxbivTv5Z-2XPf0m86wsebNIG15pyUHojzmRvJKySNwfAHs7sprTGsA_SIR_H43h";


Nice, this is a great use of constants here!

synthead

Got a couple more suggestions, mostly about:

Using $config->val( ... )'s default parameter.
Using a %defaults nested hash.
Using semantic versioning.

Also, if you would like, please feel free to add me as a collaborator if you'd like to work together on PRs a little easier! You can protect the master branch from direct pushes, and require that pull requests get an approval from you before allowing a merge to master. Safety first! 😉

synthead · 2018-04-02T14:01:15Z

README.md


 ### Making sure everything is running (in manual mode)
-* Start the event server manually first using `sudo -u www-data /usr/bin/zmeventnotification.pl` and make sure you check syslogs to ensure its loaded up and all dependencies are found. If you see errors, fix them. Then exit and follow the steps below to start it along with Zoneminder. Note that the `-u www-data` runs this command with the user id that apache uses (in some systems this may be `apache` or similar). It is important to run it using the same user id as your webserver because that is the permission zoneminder will use when run as a daemon mode.
+* I am assuming you have downloaded the 2 files to your current directory in the step below
+* Start the event server manually first using `sudo -u www-data ./zmeventnotification.pl --config ./zmeventnotification.ini` (Note that if you omit `--config` it will look for `/etc/zmeventnotification.ini` and if that doesn't exist, it will use default values) and make sure you check syslogs to ensure its loaded up and all dependencies are found. If you see errors, fix them. Then exit and follow the steps below to start it along with Zoneminder. Note that the `-u www-data` runs this command with the user id that apache uses (in some systems this may be `apache` or similar). It is important to run it using the same user id as your webserver because that is the permission zoneminder will use when run as a daemon mode.


synthead · 2018-04-02T14:07:46Z

zmeventnotification.pl

@@ -58,74 +51,286 @@
 # ==========================================================================


-my $app_version="0.98.5";
+my $app_version="1.0";


If you haven't picked it up yet, this would be a great time to use semantic versioning. From the site:

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.

Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

Since this PR would include a breaking change, this would be a great time to stamp it at 1.0.0.

Also, you should tag this as a 1.0.0 release in GitHub! This means that users can use a Seal of Approval ™️ stable version instead of pulling from master. If anything, that means that I can package up a stable version, too 👀

synthead · 2018-04-02T14:31:53Z

zmeventnotification.pl

+use constant DEFAULT_CUSTOMIZE_MONITOR_RELOAD_INTERVAL => 300;
+use constant DEFAULT_CUSTOMIZE_READ_ALARM_CAUSE => 0;
+use constant DEFAULT_CUSTOMIZE_TAG_ALARM_EVENT_ID => 0;
+use constant DEFAULT_CUSTOMIZE_USE_CUSTOM_NOTIFICATION_SOUND => 0;


These constants have super long variable names. Maybe this would be the right time to utilize nested hashes?

my %defaults = ( network => { port => 9000 }, auth => { enable => 1, timeout => 20, }, fcm => { enable => 1, token_file => '/etc/private/tokens.txt', }, ssl => { enable => 1, }, customize => { verbose => 0, event_check_interval => 5, monitor_reload_interval => 300, read_alarm_cause => 0, tag_alarm_event_id => 0, use_custom_notification_sound => 0, } );

Then you can reference a value by $defaults{network}{port}.

Alternatively, we could just add a comment above the $config->val( ... ) section that says that the default values are read in that spot.

synthead · 2018-04-02T14:36:02Z

zmeventnotification.pl


+#default key. Please don't change this
+use constant NINJA_API_KEY => "AAAApYcZ0mA:APA91bG71SfBuYIaWHJorjmBQB3cAN7OMT7bAxKuV3ByJ4JiIGumG6cQw0Bo6_fHGaWoo4Bl-SlCdxbivTv5Z-2XPf0m86wsebNIG15pyUHojzmRvJKySNwfAHs7sprTGsA_SIR_H43h";


I think this should be added to the "default" lines above (or in the hash I suggested).

synthead · 2018-04-02T14:36:37Z

zmeventnotification.pl


+#default key. Please don't change this


At this point, users shouldn't be editing this script unless they have their own specific needs outside of configuration, so we should be good omitting this 😉

synthead · 2018-04-02T14:43:26Z

zmeventnotification.pl

+
+# this is just a wrapper around Config::IniFiles val
+# older versions don't support a default parameter
+sub config_get_val {


Default values are integrated into Config::IniFiles, so we shouldn't need to write our own code to do this. Just pass the default as the optional third parameter to $config->val( ... ) and there you have it. In fact, by the way you formatted this, you could simply replace the config_get_val( ... ) calls in lines 234-252 with $config->val( ... ).

You had mentioned in a comment earlier that the default values weren't working for some reason, but I can verify that they are working here. What error messages were you seeing with my original PR? What were the blockers that led you to write this bit?

In my tests, the defaults were never being assigned even if the parameter was absent in the config

synthead · 2018-04-02T14:46:31Z

zmeventnotification.pl

+$monitor_reload_interval       //= config_get_val($config, "customize", "monitor_reload_interval",       DEFAULT_CUSTOMIZE_MONITOR_RELOAD_INTERVAL);
+$read_alarm_cause              //= config_get_val($config, "customize", "read_alarm_cause",              DEFAULT_CUSTOMIZE_READ_ALARM_CAUSE);
+$tag_alarm_event_id            //= config_get_val($config, "customize", "tag_alarm_event_id",            DEFAULT_CUSTOMIZE_TAG_ALARM_EVENT_ID);
+$use_custom_notification_sound //= config_get_val($config, "customize", "use_custom_notification_sound", DEFAULT_CUSTOMIZE_USE_CUSTOM_NOTIFICATION_SOUND);


These lines are starting to get really long. Maybe break them up into multiple lines? Note: I used $config->val( ... ) and the %defaults hash that follows my other suggestions in this example.

$use_custom_notification_sound //= $config->val( $config, "customize", "use_custom_notification_sound", $defaults{customize}{use_custom_notification_sound} );

pliablepixels · 2018-04-02T15:14:28Z

@synthead - added you as a collaborator. Please feel free to modify this PR. I'm ok with all your suggested edit (including hash), except the item mentioned below.

For now, please don't remove the wrapper default function. On my system, the default parameter is completely ignored in $config(a,b,def) -> even if the ini file has 'a' and no b.

synthead and others added 23 commits March 29, 2018 10:32

Read configuration from an INI file.

3fa4a43

Move event_check_interval and monitor_reload_interval to customize se…

641129e

…ction.

Fix incorrect INI sections for a couple options.

408dad9

Add option to specify INI file location.

9776fbf

Add CLI option support.

887ea5e

Use "enable" for some options.

16ac4e7

Add --check-config option.

ddb9679

Rename "log_to_console" to "verbose".

54a48e4

Add inverse flags for all boolean options.

b0bbf94

Rename log_to_console to verbose in INI file.

9449f0a

Use "Verbose" in print_config.

94a2718

Add --help page.

6b429a8

Update read_alarm_cause comment in INI file.

d549725

Disable verbose mode by default.

2ff1f70

Put FCM API key in INI and options.

f63277f

Enable SSL by default.

b24fbff

Update README.md to refer to the configuration file.

e723e11

Merge branch 'master' of github.com:pliablepixels/zmeventserver into …

f6e6a78

…support-options-from-cli-and-config-file

changed defaults so as not to break old paths

ff20a79

use inbuilt fcm key if none specified, fixed some defaults for backwa…

2560d33

…rd compat

changed config_file_path to _file

c27f372

removed quotes

283cb29

default doesn't seem to be working at all in ->val?

6164b15

pliablepixels added 2 commits March 31, 2018 17:02

instructions

411d71f

instructions

7d6b074

pliablepixels mentioned this pull request Mar 31, 2018

Rework zmeventserver initialization, move to ini file #42

Closed

instructions

ed69899

synthead suggested changes Mar 31, 2018

View reviewed changes

pliablepixels added 4 commits April 1, 2018 09:29

various updates based on review comments

15dce34

auth enabled updated

28d73f7

moved File:: to try_use as well

58c4533

move app defaults to constants

26f6f4d

synthead suggested changes Apr 2, 2018

View reviewed changes

added clarification for verbose

4d31801

pliablepixels merged commit bfd3aa4 into master Apr 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synthead support options from cli and config file #41

Synthead support options from cli and config file #41

pliablepixels commented Mar 31, 2018

pliablepixels commented Mar 31, 2018

pliablepixels commented Mar 31, 2018 •

edited

synthead left a comment

synthead Mar 31, 2018

synthead Mar 31, 2018

synthead Mar 31, 2018

synthead Mar 31, 2018

pliablepixels Apr 1, 2018

synthead Mar 31, 2018

synthead Mar 31, 2018

synthead Mar 31, 2018

synthead Mar 31, 2018 •

edited

synthead Mar 31, 2018

synthead Mar 31, 2018

synthead left a comment

synthead Apr 2, 2018

synthead Apr 2, 2018

synthead Apr 2, 2018

synthead Apr 2, 2018

synthead Apr 2, 2018

synthead Apr 2, 2018

pliablepixels Apr 2, 2018 •

edited

synthead Apr 2, 2018

pliablepixels commented Apr 2, 2018 •

edited


		--help Print this page.

		--config=FILE Read options from configuration file (default: /etc/zmeventnotification.ini).


		#default key. Please don't change this
		use constant NINJA_API_KEY => "AAAApYcZ0mA:APA91bG71SfBuYIaWHJorjmBQB3cAN7OMT7bAxKuV3ByJ4JiIGumG6cQw0Bo6_fHGaWoo4Bl-SlCdxbivTv5Z-2XPf0m86wsebNIG15pyUHojzmRvJKySNwfAHs7sprTGsA_SIR_H43h";

Synthead support options from cli and config file #41

Synthead support options from cli and config file #41

Conversation

pliablepixels commented Mar 31, 2018

pliablepixels commented Mar 31, 2018

pliablepixels commented Mar 31, 2018 • edited

synthead left a comment

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

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

Choose a reason for hiding this comment

synthead Mar 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

synthead left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pliablepixels Apr 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pliablepixels commented Apr 2, 2018 • edited

pliablepixels commented Mar 31, 2018 •

edited

synthead Mar 31, 2018 •

edited

pliablepixels Apr 2, 2018 •

edited

pliablepixels commented Apr 2, 2018 •

edited