-
Notifications
You must be signed in to change notification settings - Fork 111
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
reorganize into app/ and server/ #88
Conversation
for lack of a better idea. ultimately i want the handlers to not reference the api package, so that the api package can reference the handlers.
routing is moving
@mohammed90 Feedback would be appreciated in light of #83. I'm imagining that the gRPC logic would be a sibling of |
Okay, I'll take a look by next weekend. I'm currently down with flu. |
Okay, no rush! |
@cainlevy LGTM. I just have one thought: In server/sessions/util.go, the getters explicitly rely on *http.Request. In gRPC side of work, I need exactly the same logic, but we don't have *http.Request there. So I copied those functions into the grpc package and adjusted funcs to take context.Context instead of *http.Request, which makes it agnostic to the source of the context.Context. Do you think it's better to refactor this logic to be reusable across both packages in this PR, or do you want to leave it for later when we're reconciling them? The setter seems to have to be tied to the response type, so I can't think of a way to make it agnostic. authn-server/server/sessions/util.go Line 27 in f08fcf9
|
@mohammed90 I'm back from the winter holiday and ready to finish up this refactor!
Let's see how it turns out in the end? The amount of duplication you're describing sounds acceptable to me. |
The new organization has three main divisions:
app/
contains domain logicserver/
maps the domain logic into a standard RESTful server using middleware and handlerslib/
contains a few generalized things that haven't been fully extracted to standalone repositories