-
Notifications
You must be signed in to change notification settings - Fork 21
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
Support location zones and resolver metrics in go client #31
Conversation
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.
Client updates looks pretty good.
Tests need to be completed before merging.
Please add a note in the PR description regarding:
- APIVersion upgrade
4
->5
- NGINX Plus version upgrade
R18
->R19
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.
@b-v-r please see my comments
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.
Looks pretty good now. Just unit tests need improvement
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.
Looks good now 👍
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 think the resolvers test could be extended to validate metrics are correctly marshalled and returned.
Something like:
if resolver, ok := stats.Resolvers[resolverMetric]; ok {
if resolver.Requests.Name < 1 {
t.Errorf("Resolvers stats missing: %v", resolver.Requests)
}
} else {
t.Errorf("Resolver %v not found", resolverMetric)
}
The current test of configuring one resolver and testing for the existence/absence of that resolver could be considered sufficient.
I defer to your judgement. LGTM.
9840cf8
to
2d0f83b
Compare
Proposed changes
Support the newly added per location and resolver metrics.
APIVersion upgrade 4 -> 5
NGINX Plus version upgrade R18 -> R19
Checklist
Before creating a PR, run through this checklist and mark each as complete.