Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Alerting: Add method for resetting notification policies #98

Merged
merged 4 commits into from
Jul 12, 2022

Conversation

alexweav
Copy link
Contributor

@alexweav alexweav commented Jul 7, 2022

Adds a method for "deleting" notification policies. Policies cannot actually be deleted - this resets the policy back to the default value.

@@ -118,3 +118,7 @@ func (c *Client) SetNotificationPolicy(np *NotificationPolicy) error {
}
return c.request("PUT", "/api/v1/provisioning/policies", nil, bytes.NewBuffer(req), nil)
}

func (c *Client) ResetNotificationPolicy() error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe ClearNotificationPolicy or DeleteNotificationPolicy? does anyone have name preferences here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't go with DeleteNotificationPolicy. I'm fine with any of the other two options :)

I don't see any identifiers, does this method reset all notification policies? If so, wouldn't it be better to use the plural form "policies" instead of "policy"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It resets the entire tree of notification policies. So, it's really ambiguous english-wise... is it changing many objects (each node of the tree) or is it changing just one (the tree itself)?

I like keeping it singular because SetNotificationPolicy and GetNotificationPolicy operates on a single object and not a slice. It's consistent, IMO, GetX should return X and GetXs should return []X.

Thoughts on using the term PolicyTree as a disambiguator? ResetNotificationPolicyTree does not have this problem.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love ResetNotificationPolicyTree actually! Not a dealbreaker though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to Tree everywhere. This is safe to do because the other methods have not been released yet.

@alexweav alexweav force-pushed the alexweav/notif-policy-reset branch from 99d2527 to 158ee7b Compare July 12, 2022 17:02
@alexweav alexweav merged commit 9bc48c7 into master Jul 12, 2022
@alexweav alexweav deleted the alexweav/notif-policy-reset branch July 12, 2022 17:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
2 participants