Conversation
fisx
left a comment
There was a problem hiding this comment.
it just occurred to me that the nginz changes are a lot or work but pretty straight-forward. just add a (/v(\d+))? in front of every path and make them all regexps. but everybody except me probably already thought of that. :) https://xkcd.com/564/
Yep, that's what I've been doing. |
|
You should also have a flag in the server config(s) that keeps supported versions from being advertised, so we can allow on-prem backends to upgrade without forcing them to make their network infrastructure federation-ready: (not sure about the name) |
823780f to
5b84cc4
Compare
I don't follow. Version 1 can be supported without having to enable federation or make any changes to the infrastructure. This change has not much to do with federation, really. |
5b84cc4 to
9ca8c66
Compare
This rewrites requests from `/vN/path` to `/path`.
Simply changing the `pathInfo` field is not enough, because some Wai applications or middleware are relying on the `rawPathInfo` field.
9ca8c66 to
a6c6d32
Compare
| import Data.Schema | ||
| import Imports | ||
|
|
||
| vinfoSchema :: ValueSchema NamedSwaggerDoc v -> ValueSchema NamedSwaggerDoc [v] |
There was a problem hiding this comment.
Why is this not in Wire.API.Routes.Version?
There was a problem hiding this comment.
VersionInfo is more general, and can be used for other API version lists, whereas Wire.API.Routes.Version is specific for the client API.
There was a problem hiding this comment.
What about Version, then? Will there be a type Version for the federation server-to-server API as well?
There was a problem hiding this comment.
Yes, that was the idea, as well as for other APIs that need to be versioned separately.
| import qualified Wire.API.Routes.Public.LegalHold as LegalHoldAPI | ||
| import qualified Wire.API.Routes.Public.Spar as SparAPI | ||
| import qualified Wire.API.Routes.Public.Util as Public | ||
| import Wire.API.Routes.Version |
There was a problem hiding this comment.
qualify this, and call versionSwagger swaggerDoc?
There was a problem hiding this comment.
I don't like to create name clashes on purpose and resolve them with qualified imports. It makes everything more awkward and I don't see the benefit.
There was a problem hiding this comment.
I won't insist, but I think we agreed at some point (we should document these things) that imports should either be explicit or qualified, but not neither. Doesn't have to happen now, since we're not consistent, but can you get on board with the general idea?
There was a problem hiding this comment.
Well, I disagree with the general idea, but if it's something everyone else agrees on, I'll comply.
This PR implements the bare minimum interface for client API version negotiation.
The changes in the nginz chart are based on https://github.com/zinfra/cailleach/pull/879 and result in the following diff.
Tracked by https://wearezeta.atlassian.net/browse/FS-440.
Checklist
changelog.d.