Add authentication to the ntfy hook #644

Tom-Hubrecht · 2023-02-24T15:58:48Z

Tom-Hubrecht commented

2023-02-24 15:58:48 +00:00

Adds basic auth to the ntfy hook.

Closes #621

Adds basic auth to the ntfy hook. Closes #621

Tom-Hubrecht requested review from witten 2023-02-24 16:01:05 +00:00

Tom-Hubrecht force-pushed ntfy-auth from a79c0b8441 to d80e716822

2023-02-24 16:36:02 +00:00

Compare

witten requested changes 2023-02-24 17:01:10 +00:00

witten left a comment

This looks great! Thanks so much for taking the time to implement it. Would you also mind adding relevant unit tests as appropriate? The ntfy test file is at tests/unit/hooks/test_ntfy.py and you can read about running borgmatic tests here: https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/#automated-tests

This looks great! Thanks so much for taking the time to implement it. Would you also mind adding relevant unit tests as appropriate? The ntfy test file is at `tests/unit/hooks/test_ntfy.py` and you can read about running borgmatic tests here: https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/#automated-tests

borgmatic/hooks/ntfy.py

						
				@ -59,0 +61,4 @@

				        auth = (

				            requests.auth.HTTPBasicAuth(username, password)

				            if (username and password) is not None

witten commented

2023-02-24 16:58:16 +00:00

This logic looks completely correct, but it was initially confusing for me. Not a big deal, just letting you know in case you want to clarify it.

Tom-Hubrecht commented

2023-02-25 19:05:59 +00:00

I made it explicit

Tom-Hubrecht marked this conversation as resolved

Tom-Hubrecht force-pushed ntfy-auth from 1c7ad67c8c to 95575c3450

2023-02-25 19:04:47 +00:00

Compare

Tom-Hubrecht requested review from witten 2023-02-25 19:06:15 +00:00

witten approved these changes 2023-02-25 22:03:55 +00:00

witten left a comment

Thanks so much for adding tests!

borgmatic/hooks/ntfy.py

						
				@ -59,0 +67,4 @@

				            logger.warn(

				                f'{config_filename}: Password missing for Ntfy authentication, defaulting to no auth'

				            )

				        elif password is not None:

witten commented

2023-02-25 22:02:50 +00:00

I like that you've split this out with warnings. But this logic might be a little more intuitive if it was something like:

if username is None:
   ...  # warn about missing username
elif password is None:
   ...  # warn about missing password
else:
   ...  # do basic auth

I like that you've split this out with warnings. But this logic might be a little more intuitive if it was something like: ```python if username is None: ... # warn about missing username elif password is None: ... # warn about missing password else: ... # do basic auth ```

witten commented

2023-02-25 22:22:30 +00:00

Although I will say that the way you did it has the distinct benefit of skipping the warning if both username and password are missing!

Although I will say that the way you did it has the distinct benefit of skipping the warning if *both* username and password are missing!

👍 1

witten marked this conversation as resolved

witten merged commit 783a6d3b45 into master

2023-02-25 22:04:39 +00:00

witten referenced this issue from a commit

2023-02-25 22:04:40 +00:00

Add authentication to the ntfy hook (#621).

witten commented