-
Notifications
You must be signed in to change notification settings - Fork 25
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 upstream parameters in nginx-asg-sync #30
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.
Thanks.
Looks good, I have added a few comments/suggestions.
Additionally, we need to add documentation of the new fields here:
https://github.com/nginxinc/nginx-asg-sync/blob/master/examples/aws.md
and here:
https://github.com/nginxinc/nginx-asg-sync/blob/master/examples/azure.md
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.
Hi @b-v-r sorry I missed one thing about max_fails
last time I reviewed it, but I think is important we take care of.
Also I suggest to add default values in docs (check my suggestion). Thanks again!
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 better now thanks!
PS: This PR can't be merged until we update the UpdateHTTP
method of the go client. Right now it does not perform updates on the Upstream parameters.
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, just some small comments regarding test style/consistency and structure.
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.
The leading spaces in cmd/sync/main_test.go
still look strange to me.
For example:
t.Errorf(" setPositiveInt()
-> t.Errorf("setPositiveInt()
Aside from that, this looks ready. - Just need updated UpdateHTTP
method from go client to proceed.
Closing in favour of #33 |
Proposed changes
As a user, I would like to configure upstream server parameters for both http and stream upstreams.
Checklist
Before creating a PR, run through this checklist and mark each as complete.