-
Notifications
You must be signed in to change notification settings - Fork 25
Small fixes to the NGINX Plus client #4
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
Conversation
- Add Total responses to upstream struct - Add json tag to HealthChecks property so it parses - Update comment
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.
(If you add a 5s sleep before fetching upstream stats you can see the HealthChecks populated)
5 seconds seems to much. what about 100ms?
client/nginx_client.go
Outdated
@@ -108,6 +108,7 @@ type Responses struct { | |||
Responses3xx uint64 `json:"3xx"` | |||
Responses4xx uint64 `json:"4xx"` | |||
Responses5xx uint64 `json:"5xx"` | |||
Total uint64 `json:"total"` |
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.
is json:"total"
requried?
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 so weird without it
client/nginx_client.go
Outdated
@@ -12,7 +12,7 @@ import ( | |||
// APIVersion is a version of NGINX Plus API. | |||
const APIVersion = 2 | |||
|
|||
// NginxClient lets you add/remove servers to/from NGINX Plus via its API. | |||
// NginxClient lets you access NGINX Plus via its API. |
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.
NginxClient lets you access NGINX Plus via its API. -> NginxClient lets you access NGINX Plus API.
imho
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.
ok
I haven't added a sleep, and one isn't necessary imho, the test is fine with 0 values in the healthcheck. |
- Fix comment - Fix inconsistent test error string case
👍 |
(If you add a 5s sleep before fetching upstream stats you can see the HealthChecks populated)