+ Add UptimeRobot all monitors features#632
Conversation
ghost
mentioned this pull request
| } | ||
|
|
||
| return nil, nil | ||
| } else if response.StatusCode == Http.StatusTooManyRequests { |
There was a problem hiding this comment.
I'm not a maintainer or anything but I'm interested in the addition of the missing UptimeRobot features, thanks for the PR!
I'm just wondering, why did you remove the code to handle HTTP 429 response status?
There was a problem hiding this comment.
I have rollback the missing parts in the last commit.
Thanks @eneiss for your review !
I hope someone can help these features be part of the project asap.
Best regads,
Didier
|
Up ! Is someone available to review this ? |
|
Up ? |
|
@dsegurakaliop I'm trying to use your PRs (but unfortunatly I'm not a maintainer so I can't help that much on merging this)
Eager to see this PR merged @MuneebAijaz ? |
| // Alert Contacts (Optional) | ||
| if providerConfig != nil && len(providerConfig.AlertContacts) != 0 { | ||
| body += "&alert_contacts=" + url.QueryEscape(providerConfig.AlertContacts) | ||
| } else { | ||
| body += "&alert_contacts=" + url.QueryEscape(monitor.alertContacts) |
There was a problem hiding this comment.
To be removed (duplicates of lines 231-235), generates an error: "Error: "alert_contacts" must be a string"
| // Interval (Optional, in seconds) | ||
| if providerConfig != nil && providerConfig.Interval > 0 { | ||
| body += "&interval=" + strconv.Itoa(providerConfig.Interval) | ||
| } else { | ||
| body += "&interval=" + strconv.Itoa(DefaultInterval) | ||
| } |
There was a problem hiding this comment.
To be removed (duplicates of lines 237-242), generates an error: "Error: "interval" must be a string"
|
Hi, will hopefully take a look at the PR this week. Thanks for the contribution |
Felix-Stakater
left a comment
Felix-Stakater
left a comment
There was a problem hiding this comment.
A couple of small changes are required before we can merge this, thanks for your contribution! :)
| } | ||
|
|
||
| // Interval (Optional, in seconds) | ||
| if providerConfig != nil && providerConfig.Interval > 0 { |
There was a problem hiding this comment.
As @dguihal commented we need to remove this block
| // Alert Contacts (Optional) | ||
| if providerConfig != nil && len(providerConfig.AlertContacts) != 0 { | ||
| body += "&alert_contacts=" + url.QueryEscape(providerConfig.AlertContacts) | ||
| } else { | ||
| body += "&alert_contacts=" + url.QueryEscape(monitor.alertContacts) |
|
Big thanks for this active review and fixes. Sorry if I have done some mistakes, I really want these features appears one day, and I think we are now very close ! Thanks everyone. |
|
Hi @dsegurakaliop I created a PR on your fork https://github.com/dsegurakaliop/IngressMonitorController/pull/1 I've managed to make this code run properly on a dev env |
|
Hey @dguihal thanks for effort. Can you do us a favor and open pull request towards us so it can be reviewed, tested and possibly merged as enhancement? |
|
Hi @mahmadmujtaba I created #655 |
ghost
closed this by deleting the head repository
5 participants
If someone can test the integration of these new features, it will be really GREAT !
I have done all I can from my little experience in GO world.
Please be free to add your stone !