Skip to content
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

Move websocket headers to opt function 'WithWebsocketHeaders' #365

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented Dec 1, 2024

Follow-up up on the discussion in #360 (review).

Move websocket headers to and opt function 'WithWebsocketHeaders'.

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Dec 1, 2024

@benjaminjkraft Here is a follow-up PR regarding our discussion here: #360 (review)

This is without tests now, which is not great.

I'm struggling to find an easy way to test this. Normally I would set up a middleware and set the authentication ctx key for the header. But it seems that the httptest server does not have middleware support out-of-the-box. Maybe I'm wrong?

Could be wise to hold off merging this until tests have been added 🤗

@benjaminjkraft
Copy link
Collaborator

Thanks!

I think you should be able to do middleware? gqlgenServer is just an http.Handler, so if you have a middleware that's func(http.Handler) http.Handler you can just do NewServer(middleware(gqlgenServer)).

@HaraldNordgren HaraldNordgren force-pushed the websocket_headers branch 2 times, most recently from 457eaeb to 4162da1 Compare December 2, 2024 00:20
@HaraldNordgren
Copy link
Contributor Author

@benjaminjkraft Thanks! Tests added now!

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks!

@benjaminjkraft benjaminjkraft merged commit d3e516b into Khan:main Dec 2, 2024
4 checks passed
@HaraldNordgren HaraldNordgren deleted the websocket_headers branch December 2, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants