-
Notifications
You must be signed in to change notification settings - Fork 7
Expose a Register function that can be used to install routes in subrouter #511
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
base: master
Are you sure you want to change the base?
Conversation
729980a to
42e5049
Compare
|
I am untagging myself here so I don't get prompted by slack everyday to review, but feel free to re-tag me at any point if you are ready for a review. |
andruwm
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 code changes here look reasonable. It's be helpful to add another swagger file which uses this new feature in samples so that its easier to understand how both the client and server look on a simple app.
| @@ -0,0 +1,7 @@ | |||
| package wagclientlogger | |||
|
|
|||
| type NoOpLogger struct{} | |||
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.
We should have had one of these a long time ago.
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.
yeah I think this would be worth cherry-picking into its own PR, probably?
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.
Agreed
I just looked through the sample PR provided and was able to get a good idea of it there, but having it all in one place would be awesome at some point. Doesnt need to be now. |
f78c605 to
93c35c4
Compare
…outer
Takes the existing logic for generating the routes in gen-go/server and applies
them to an injected router. This allows a server to compose routers by calling
```
Register(parentRouter.PathPrefix("/subrouter-prefix").Subrouter(), controller)
```
Testing this hypothesis out in app-district-service.
The extension schema is of the form
```yaml
x-routers:
- key: subrouter1
path: /v1/abc
- key: subrouter2
path: /v1/xyz
```
This pattern assumes that the repo has a routers/ directory with subdirectories
routers/subrouter1 and routers/subrouter2, with both being subdirectories having
wag-generated route handlers.
…g subrouter This change allows us to use the existing notion of `basePath` to generate the correct client paths for a subrouter, assuming that we do not want to support path parameters for a subrouter, which keeps us spec compliant for OAS 2.
Since the strategy for subrouter JavaScript clients (at least, so far), has been to include the subrouters in the root client generation — which is the opposite of how the Go clients are generated — we don't need to generate the subrouter clients. This change autosets conf.generateJSClient to false if conf.subrouter is true.
93c35c4 to
7edc879
Compare
Expose a Register function that can be used to install routes in subrouter
Takes the existing logic for generating the routes in gen-go/server and applies them to an injected router. This allows a server to compose routers by calling
Testing this hypothesis out in app-district-service.
Jira
SHAPI-1697
Testing
See Clever/app-district-service#2.
Checklist
make buildmake generate/VERSIONfile. First line should be JUST the version e.g.v10.13.1. Then a blank line. Then release notes.