-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
bluecat: improve test coverage #2185
bluecat: improve test coverage #2185
Conversation
We were returning an error when the owner string was not found and this was causing the loop to quit. This is not ideal because it does not fetch the other records once it returns.
/assign @seanmalloy |
@@ -215,7 +215,7 @@ func (p *BluecatProvider) Records(ctx context.Context) (endpoints []*endpoint.En | |||
tempEndpoint := endpoint.NewEndpoint(rec.Name, endpoint.RecordTypeTXT, rec.Properties) | |||
tempEndpoint.Labels[endpoint.OwnerLabelKey], err = extractOwnerfromTXTRecord(rec.Properties) |
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.
Since the only error being returned by extractOwnerfromTXTRecord
is if an owner is not found, and not having an owner is a valid thing that can happen (ex if something was created in Bluecat by something other than External DNS), it might make sense to just not return an error on the function.
Another option could be to get the value and do not directly attempt to set the label of the Endpoint, and then if an error is not returned in the function, the endpoint would have the label added, and it would be added to the list of endpoints.
@seanmalloy what if your opinion on that?
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.
After talking with Sean, we think this is the appropriate way to handle this
/kind cleanup |
/lgtm |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaideepbellani, vinny-sabatini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Removed return statements for error from BluecatRecords function.
Added two new tests and test cases to improve test coverage
Checklist