-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add Additional Upstream Server Parameters #30
Add Additional Upstream Server Parameters #30
Conversation
8151889
to
e68bf3a
Compare
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.
Please add unit test coverage for all new Upstream Server parameters added in this PR.
Please note the use of omitempty
should only be used where a default must be set from the nplus api. Otherwise I don't think omitempty
should be used.
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. Apart from adding tests as @Dean-Coakley suggested, I think the following:
Backup bool `json:"backup,omitempty"`
Down bool `json:"down,omitempty"`
Drain bool `json:"drain,omitempty"`
should be pointers.
The reason is, if you use omitempty
in Go with a bool. If the user sets that bool to False
the JSON will skip that field, and we can't really do that. Changing those to *bool
will make the difference, because now empty is nil
and true/false
can be valid options if the user wants to fill that.
Does this make sense ?
PS: Alternatively you could remove the omitempty
which is what you did I think, because those parameters are false by default.
Regarding Update: |
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 to me 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.
Looks good to me, I think we can add Service
tests if we use the test.conf
that already uses a resolver. Let me know what you think.
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.
@Rulox re #30 (review) , not sure if it is worth doing. For a proper test, in addition to the resolver in the config, we will have to have a resolver that returns something for a DNS query.
tests/client_test.go
Outdated
servers[0].Drain = true | ||
|
||
// Updating existing upstream server with drain directive | ||
_, _, err = c.UpdateHTTPServers(upstream, servers) |
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.
UpdateHTTPServers
doesn't update the existing upstream server - it can only add or delete, but not update. As a result, this test is not meaningful.
If we want to have a proper test, the client needs to have a method that updates an existing server. This method will send a PATCH request to the API http://nginx.org/en/docs/http/ngx_http_api_module.html#http_upstreams_http_upstream_name_servers_http_upstream_server_id to update an upstream server. However, this should not be in the scope of this tasks. For one thing, that new method will require new tests as well.
To fix this test, I suggest the following:
- Add a test upstream to
test.conf
:
upstream test-drain {
zone test-drain 64k;
server 127.0.0.1:9001 drain;
}
- Change the test so that:
a. the test gets the servers of that upstreams
b. it checks that the number of upstream servers is 1
c. it compares the upstream server with the expected. The expected should have drain =true
.
This way the updated test checks that if the drain is set to true in the API, the client will see the same value in the API response. However, it doesn't check that the client can set drain true
, because of the mentioned limitation above.
Yeah I was thinking of that too @pleshakov |
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.
👍
8d423dc
to
9333b5a
Compare
Fixes #28
Proposed changes
Extend the go client to support additional parameters in upstream servers:
route
backup
down
drain
weight
service
Make sure that when adding an upstream server with unspecified parameters, the values of those parameters in NGINX Plus API are set with their default values.
Checklist
Before creating a PR, run through this checklist and mark each as complete.