-
Notifications
You must be signed in to change notification settings - Fork 28
Add upstream parameters in nginx-asg-sync #30
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
Rulox
left a 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.
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!
Rulox
left a 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.
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.
Dean-Coakley
left a 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.
Looks good, just some small comments regarding test style/consistency and structure.
Dean-Coakley
left a 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.
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.