-
Notifications
You must be signed in to change notification settings - Fork 2k
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 retries support for vs/vsr #628
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.
@LorcanMcVeigh thanks for the PR!
Please see my comments.
Additionally, we need to validate all fields in our resources. Make sure to add validation for the newly added fields. You can take a look at how other fields in the upstream are validated. next_upstream
might be tricky - make sure to study carefully how it works http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream As always, make sure you validation code works the same way as NGINX would validate those fields.
examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml
Outdated
Show resolved
Hide resolved
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.
lgtm, 1 small suggestion. Also as Michael said, you need to add validation for the new fields. Check https://github.com/nginxinc/kubernetes-ingress/blob/master/pkg/apis/configuration/validation/validation.go#L124
e17ed0c
to
742676a
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.
Thanks!
Couple of suggestions for the new changes
dacd4a7
to
9ebcc9b
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.
Looks good , few questions/suggestions!
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.
@LorcanMcVeigh Please see additional 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.
Approving! Before merging, please fix the remaining small suggestions.
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.
lgtm @LorcanMcVeigh remember to squash all your commits in one before merging.
Proposed changes
Added support and documentation for next-upstream-timeout and next-upstream-retries
Checklist
Before creating a PR, run through this checklist and mark each as complete.