Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Feature/advanced error handling #294
Conversation
merge latest
Verified
…stem and pingdom monitor with method that prevents duplicated check creation
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 1 has Failed! |
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 2 has Failed! |
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 3 has Failed! |
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 4 has Failed! |
Verified
…sn't exist
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 5 has Failed! |
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 6 has Failed! |
Verified
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 7 has Failed! |
|
Strange thing, because all tests have been passed locally.
|
Verified
Verified
This reverts commit 4684028.
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 8 has Failed! |
Verified
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 9 has Failed! |
|
Linter, tests, and builds are fine |
|
@artemlive Yikes! You better fix it before anyone else finds out! Build 10 has Failed! |
|
Hi @artemlive, everything was fine but the test cases were failing against a different provider. I am looking into it and hopefully will resolve that today. |
|
Hi, please merge the latest master in your branch. Also, there are a fw issues with the PR:
|
| monitor, err := findMonitorByName(r.monitorServices[index], monitorName) | ||
| // if there was some error while getting a monitor, re-queue it to try on the next reconcile iteration | ||
| if err != nil { | ||
| return reconcile.Result{RequeueAfter: RequeueTime}, err | ||
| } |
ahmedwaleedmalik
Nov 12, 2020
Member
If it's unable to create a monitor this will be an endless re-conciliation loop !
If it's unable to create a monitor this will be an endless re-conciliation loop !
artemlive
Nov 12, 2020
Author
Contributor
But if it was only the API call error? E.g. API has returned 500 code, and we would be able to add this monitor on the next iteration?
But if it was only the API call error? E.g. API has returned 500 code, and we would be able to add this monitor on the next iteration?
| if err != nil { | ||
| log.Errorf("Error while handling update: %s", err) | ||
| return reconcile.Result{RequeueAfter: RequeueTime}, err | ||
| } |
ahmedwaleedmalik
Nov 12, 2020
Member
This is redundant since it already requeue's after create or update action:
https://github.com/stakater/IngressMonitorController/blob/master/pkg/controller/endpointmonitor/endpointmonitor_controller.go#L135
This is redundant since it already requeue's after create or update action:
https://github.com/stakater/IngressMonitorController/blob/master/pkg/controller/endpointmonitor/endpointmonitor_controller.go#L135
| if err != nil { | ||
| log.Errorf("Error while handling create: %s", err) | ||
| return reconcile.Result{RequeueAfter: RequeueTime}, err | ||
| } |
ahmedwaleedmalik
Nov 12, 2020
Member
This is redundant since it already requeue's after create or update action:
https://github.com/stakater/IngressMonitorController/blob/master/pkg/controller/endpointmonitor/endpointmonitor_controller.go#L135
This is redundant since it already requeue's after create or update action:
https://github.com/stakater/IngressMonitorController/blob/master/pkg/controller/endpointmonitor/endpointmonitor_controller.go#L135
artemlive
Nov 12, 2020
Author
Contributor
Thanks, I'll fix that.
Thanks, I'll fix that.
| log.Printf("Application Insights WebTest %s was not found in Resource Group %s", monitorName, aiService.resourceGroup) | ||
| return nil, nil |
ahmedwaleedmalik
Nov 12, 2020
Member
Why are we removing this error from here ?
Why are we removing this error from here ?
| log.Printf("There is no gcloud MonitorService with name %s found.", name) | ||
| return nil, nil |
ahmedwaleedmalik
Nov 12, 2020
Member
Why are we removing this error from here ?
Why are we removing this error from here ?
| log.Printf("There is no PingdomMonitorService with name %s found.", name) | ||
| return nil, nil |
ahmedwaleedmalik
Nov 12, 2020
Member
Why are we removing this error from here ?
Why are we removing this error from here ?
| log.Printf("There is no StatusCakeMonitorService with name %s found.", name) | ||
| return nil, nil |
ahmedwaleedmalik
Nov 12, 2020
Member
Why are we removing this error from here ?
Why are we removing this error from here ?
Verified
Verified
…ntroller into feature/advanced-error-handling
|
@artemlive Image is available for testing. |
Here is the issue with the detailed description of this improvement: #293