fail on canonical zone id not detected #94
Conversation
|
👍 |
|
👍 |
consumers/aws.go
Outdated
| @@ -95,6 +96,7 @@ func (a *awsConsumer) Sync(endpoints []*pkg.Endpoint) error { | |||
| func (a *awsConsumer) syncPerHostedZone(kubeRecords []*route53.ResourceRecordSet, zoneID string) error { | |||
| existingRecords, err := a.client.ListRecordSets(zoneID) | |||
| if err != nil { | |||
| log.Errorf("failed to list records in zoneID:%s. Error: %v", zoneID, err) | |||
There was a problem hiding this comment.
nitpick: space after colon
| @@ -140,7 +140,7 @@ func validateIngress(ing extensions.Ingress, filter map[string]string) error { | |||
| switch { | |||
| case len(ing.Status.LoadBalancer.Ingress) == 0: | |||
| return fmt.Errorf( | |||
| "[Ingress] The load balancer of ingress '%s/%s' does not have any ingress.", | |||
| "[Ingress] The load balancer field of ingress '%s/%s' is empty", | |||
| @@ -147,7 +147,7 @@ func validateService(svc api.Service, filter map[string]string) error { | |||
| switch { | |||
| case len(svc.Status.LoadBalancer.Ingress) == 0: | |||
| return fmt.Errorf( | |||
| "[Service] The load balancer of service '%s/%s' does not have any ingress.", | |||
| "[Service] The load balancer field of service '%s/%s' is empty", | |||
There was a problem hiding this comment.
👍 this message was very confusing
|
LGTM |
|
👍 |
|
👍 |
| @@ -53,6 +53,7 @@ func withClient(c AWSClient, groupID string) *awsConsumer { | |||
| func (a *awsConsumer) Sync(endpoints []*pkg.Endpoint) error { | |||
| kubeRecords, err := a.endpointsToRecords(endpoints) | |||
| if err != nil { | |||
| log.Errorf("failed to convert endpoints to RRS: %v. Aborting sync...", err) | |||
| return err | |||
There was a problem hiding this comment.
In general errors should either be logged or returned, but not both.
To quote Dave Cheney:
If you choose to handle the error by logging it, by definition it’s not an error any more — you handled it. The act of logging an error handles the error, hence it is no longer appropriate to log it as an error.
:)
There was a problem hiding this comment.
yes, totally make sense. Initially I wanted to print debug message, but I thought debug is not relevant here either. If error is not logged here with additional message, then it is a bit difficult to figure where the flow is aborted
There was a problem hiding this comment.
I would wrap the returned error in fmt.Errorf with the same text from the log, and then have the caller log it.
There was a problem hiding this comment.
yup, there are few other places where it needs to be changed, but probably will do it in next PR
No description provided.