-
Notifications
You must be signed in to change notification settings - Fork 28
feat(targets): Failover targets for 3.12 #2772
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
✅ Deploy Preview for kongdeveloper ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Ran through it, had no trouble.
Do we know if decK is going to support this before release? If not, then we'll need to use curl (and add a separate command to create an Upstream before creating Targets).
If decK is going to support this soon, then we should mark the version of decK in the prereqs.
@lena-larionova I've asked eng about this and will adjust the docs depending on what I hear back. |
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.
This looks pretty good!
I have just a few comments:
- I didn't see any mention that more than one target can be used as failover. If you think it's worth adding this info, you could say their weights will be respected when choosing which one will proxy the request.
- I see the Target Schema is included from somewhere else. Is it from the code? In the rendered doc it is missing the failover field.
- I think it is worth mentioning nothing blocks the user from adding a failover target to consistent-hashing and sticky-sessions upstreams, but they will be ignored, not used in regular balancing or acting as failover.
@locao Thanks for the feedback! I've applied changes for your suggestions. For this:
We autogenerate the schema closer to release in our |
We should be able to do this now - I'll get it updated on the release branch. |
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, and pre-approved. You can merge it with decK instructions if our confidence on that is high.
Signed-off-by: Diana <[email protected]>
Co-authored-by: lena-larionova <[email protected]>
Signed-off-by: Diana <[email protected]>
Co-authored-by: lena-larionova <[email protected]>
7a6a8b0
to
14f1d92
Compare
Signed-off-by: Diana <[email protected]>
@lena-larionova It's likely decK won't support this with GA, so I've converted the failover examples to API requests. |
Broken links are expected since the pages are |
Description
Fixes #2619
Preview Links
for i in
in a request-check validation block, tried and it didn't work)For reviewers, decK doesn't support failover yet, so use this to configure your targets in the how to:
Checklist
description
entry in frontmatter.