borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
14
57
Fork 41
Code Issues 53 Pull Requests 2 Actions Packages Releases 122 Activity

Support for healthchecks auto provisionning (related to #815) #852

Merged
witten merged 4 commits from estebanthi/borgmatic:f/healthchecks-auto-provisionning into main 2024-04-23 16:23:27 +00:00
Conversation 1 Commits 4 Files Changed 3 +85
estebanthi commented 2024-04-19 08:47:36 +00:00
Contributor
Copy Link
No description provided.
estebanthi added 1 commit 2024-04-19 08:47:36 +00:00
witten requested changes 2024-04-19 16:27:52 +00:00
witten left a comment
Owner
Copy Link

Thanks for implementing this—and tests too! Overall approach looks good to me, although I left some comments about the specifics of the regex used to match ping URLs.

Thanks for implementing this—and tests too! Overall approach looks good to me, although I left some comments about the specifics of the regex used to match ping URLs.
borgmatic/hooks/healthchecks.py Outdated
@ -59,10 +60,15 @@ def ping_monitor(hook_config, config, config_filename, state, monitoring_log_lev
)
return
ping_url_is_uuid = re.match(r'(\w{4}-?){4}$', hook_config['ping_url'])
witten commented 2024-04-19 16:24:43 +00:00
Owner
Copy Link

I don't think this regular expression is actually correct for Healthchecks' ping URLs. For instance, this doesn't match:

print(re.match(r'(\w{4}-?){4}$', 'https://hc-ping.com/45ad67e7-8c9a-4dc8-a629-1a0e72f5a9ed'))
None

So maybe something like this would work better?

print(re.search(r'\w{8}-\w{4}-\w{4}-\w{4}-\w{12}$', 'https://hc-ping.com/45ad67f7-8c9a-4dc8-a629-1a0e72e5a9ed'))
<re.Match object; span=(20, 56), match='45ad67f7-8c9a-4dc8-a629-1a0e72e5a9ed'>

Note the use of re.search(), which can match against part of a string rather than having to match against the whole thing like re.match().

I don't think this regular expression is actually correct for Healthchecks' ping URLs. For instance, this doesn't match: ```python print(re.match(r'(\w{4}-?){4}$', 'https://hc-ping.com/45ad67e7-8c9a-4dc8-a629-1a0e72f5a9ed')) None ``` So maybe something like this would work better? ```python print(re.search(r'\w{8}-\w{4}-\w{4}-\w{4}-\w{12}$', 'https://hc-ping.com/45ad67f7-8c9a-4dc8-a629-1a0e72e5a9ed')) <re.Match object; span=(20, 56), match='45ad67f7-8c9a-4dc8-a629-1a0e72e5a9ed'> ``` Note the use of `re.search()`, which can match against part of a string rather than having to match against the whole thing like `re.match()`. ```
witten marked this conversation as resolved
borgmatic/hooks/healthchecks.py Outdated
@ -63,3 +66,4 @@
if healthchecks_state:
ping_url = f'{ping_url}/{healthchecks_state}'
if hook_config.get('create_slug') and not ping_url_is_uuid:
witten commented 2024-04-19 16:26:02 +00:00
Owner
Copy Link

Do you think it's worth issuing a warning or even an error if a UUID-style URL is used with create_slug? Seems like a user might want to find out about the misconfiguration instead of it silently getting swallowed.

Do you think it's worth issuing a warning or even an error if a UUID-style URL is used with `create_slug`? Seems like a user might want to find out about the misconfiguration instead of it silently getting swallowed.
witten marked this conversation as resolved
estebanthi added 3 commits 2024-04-22 18:48:18 +00:00
estebanthi added 1 commit 2024-04-22 19:01:25 +00:00
estebanthi force-pushed f/healthchecks-auto-provisionning from 48c2f2f198 to 407bb33359 2024-04-22 19:16:57 +00:00 Compare
estebanthi commented 2024-04-22 19:20:37 +00:00
Author
Contributor
Copy Link

Thanks for your feedback! I've modified the regex, and you're right it's worth issuing a warning if using a UUID-style URL with create_slug, I've implemented this too.

Thanks for your feedback! I've modified the regex, and you're right it's worth issuing a warning if using a UUID-style URL with `create_slug`, I've implemented this too.
witten approved these changes 2024-04-23 16:22:11 +00:00
witten left a comment
Owner
Copy Link

LGTM! Thanks so much for the PR and for making these updates.

LGTM! Thanks so much for the PR and for making these updates.
tests/unit/hooks/test_healthchecks.py
@ -267,0 +316,4 @@
)
def test_ping_monitor_issues_warning_when_ping_url_is_uuid_and_create_slug_true():
witten commented 2024-04-23 16:21:49 +00:00
Owner
Copy Link

IMO this test could be combined with the previous given that there setups are identical and the only assertion difference is a single log action. (Do not feel strongly.)

IMO this test could be combined with the previous given that there setups are identical and the only assertion difference is a single log action. (Do not feel strongly.)
witten merged commit 7a110c7acd into main 2024-04-23 16:23:27 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
witten
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#852
No description provided.
Delete Branch "estebanthi/borgmatic:f/healthchecks-auto-provisionning"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?