-
-
Notifications
You must be signed in to change notification settings - Fork 629
feat(alerting): ClickUp alerting provider #1462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
alerting/provider/clickup/clickup.go
Outdated
| } | ||
|
|
||
| func (provider *AlertProvider) CloseTask(cfg *Config, ep *endpoint.Endpoint) error { | ||
| fetchURL := fmt.Sprintf("https://api.clickup.com/api/v2/list/%s/task?include_closed=false", cfg.ListID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly cfg.APIURL should be used here instead of the default one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, updated, APIURL is now more generic (just the base) and other parts are now using it to construct the full URL.
alerting/provider/clickup/clickup.go
Outdated
| } | ||
|
|
||
| func (provider *AlertProvider) UpdateTaskStatus(cfg *Config, taskID, status string) error { | ||
| updateURL := fmt.Sprintf("https://api.clickup.com/api/v2/task/%s", taskID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cfg.APIURL here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
README.md
Outdated
| | `alerting.clickup` | Configuration for alerts of type `clickup` | `{}` | | ||
| | `alerting.clickup.list-id` | ClickUp List ID where tasks will be created | Required `""` | | ||
| | `alerting.clickup.token` | ClickUp API token | Required `""` | | ||
| | `alerting.clickup.api-url` | Custom API URL (optional, defaults to `https://api.clickup.com/api/v2/list/{list-id}/task`) | `""` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value should be moved to default column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
README.md
Outdated
| | `alerting.clickup.name` | Custom task name template (supports placeholders) | `""` | | ||
| | `alerting.clickup.content` | Custom task content template (supports placeholders) | `""` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented default values do not match default values in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
This provider does not support global overriding of config values based on the group the endpoint is in. I've not noticed most other alert providers (not all though) support it. @TwiN Is it a requirement that new providers support this? |
|
@PythonGermany Should I port it from the Datadog Provider? |
|
If it's not too much trouble. If you want to be sure I'd wait for feedback from TwiN as I am not sure why some providers implement it and some not. The changes required in the provider I wonder why this functionality is not implemented somewhere once like a template and then reused for every provider. |
…om/TwiN/gatus/v5/client from http.Client
|
@PythonGermany Not a big deal, just lifted it from the datadog provider. Let me know if it requires any changes! If you decide to go the template route, I'd be happy to change the provider to match that. |
alerting/provider/clickup/clickup.go
Outdated
| if override.Status != "" { | ||
| cfg.Status = override.Status | ||
| } | ||
| if override.Priority != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is priority '0' a possible valid input as override priority in ClickUp alerts? If yes, a the config priority member could be a bool pointer and then check for nil to see if it is set in the override config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, priority has to be between 1 to 4, I have added validation for it now as well in the Validate function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't priority 3 be a reasonable default then if no manual priority was set in the config or override config? What happens with the task priority if it is not set and the POST request body for to create the task has value '0' set as priority?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think you are right, setting 0 / nil both are creating a task with no priority assigned (we are using Urgent in our deployment, but I think Normal is a better default for others).
Should I make the default 3 then?
Also while 0 is working, the ClickUp docs suggests to just omit it, it might not work in the future, so should I also add a bool pointer as you suggested for folks who want to deliberately set No Priority.
What I am thinking is no-priority config option, if set, priority won't be validated or sent to ClickUp, if not set, provided priority (or default = 3) will be validated and used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I am thinking is no-priority config option, if set, priority won't be validated or sent to ClickUp
Good point, I did not think about the scenario where no priority might be wanted instead of the default. An alternative to adding another option (where we would have to explain in the docs how it impacts the other option) I would suggest to allow 0 as input for priority and just add the priority config value to the create task request body if its not nil and not 0.
so should I also add a bool pointer as you suggested for folks who want to deliberately set No Priority
Should I make the default 3 then?
Yes, making it a bool pointer and default priority Normal sounds good.
Do the people who might configure ClickUp as an alert provider usually know which priority number corresponds to which priority name?
If not it might make improve usability to set the priority option as a string and map the string values to the right number before the API call, with an additional None value input allowed which maps to null in the API call.
Sorry about all the questions. It's interesting how the devil's always in the details 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can assume they will know, so I was adding the description in the README, but I like your idea of a map a lot more, better ux + also supports the None scenario well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I have also added instructions on how to get assignee ids and made notify-all configurable (defaults to true)
Co-authored-by: PythonGermany <[email protected]>
|
GitHub mobile is being silly and won't let me submit a code review, but please remove the overrides from the example. I try to keep the examples as short as possible since the documentation is already quite lengthy . |
|
@TwiN Done |
|
GitHub mobile is still being silly, but please remove the unnecessary empty newlines inside functions, and then I think we're good to merge! |
|
@TwiN No worries, removed |
|
@TheBinaryGuy Thank you for the contribution, and great work! @PythonGermany Thank you for the assist! |
|
Thank you @PythonGermany and @TwiN and thanks for maintaining this amazing project! |
Summary
Closes #1426
Checklist
README.md, if applicable.