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

Swagger page per internal endpoint #3094

Merged
merged 11 commits into from
Feb 27, 2023

Conversation

supersven
Copy link
Contributor

@supersven supersven commented Feb 17, 2023

This PR renders a Swagger page per internal endpoint. The benefit is that we don't have to play crazy tricks to get all (overlapping!) paths right. Currently, this is solved in develop by prefixing the paths by their service name (e.g. /<brig>/i/status.)

Executing the swagger operations by clicking on Execute doesn't work and never will: The services do not handle CORS related headers. Thus, the browser refuses to accept the response. But, the rendered curl command works if kubectl forward-port is called as described.

This PR has been inspired by this comment: #3007 (comment)

And, I've removed SwaggerTag, because it's as simple to get its benefits on value level.

Rendered

image
image
image
image
image
image

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Feb 17, 2023
@supersven supersven requested review from fisx and battermann February 20, 2023 15:14
@supersven supersven marked this pull request as ready for review February 20, 2023 15:14
@fisx
Copy link
Contributor

fisx commented Feb 22, 2023

didn't look at the code, just general questions. feel free to ignore me until i've filed a proper review.

And, I've removed SwaggerTag, because it's as simple to get its benefits on value level.

why is value level superior? :) anyway, i'll read it first, then complain.

why does legalhold has its own page?

Currently, this is solved in develop by prefixing the paths by their service name (e.g. //i/status.)

so this PR undoes that solution? (no objection, just asking)

Executing the swagger operations by clicking on Execute doesn't work and never will: The services do not handle CORS related headers. Thus, the browser refuses to accept the response. But, the rendered curl command works if kubectl forward-port is called as described.

did the same limitation hold for the above solution /brig/i/status? if not why is this PR an improvement?

if we decide to live with this limitation (fine by me), we should document on docs.wire.com or (better yet) on the swagger pages in the info heading.

@supersven
Copy link
Contributor Author

supersven commented Feb 22, 2023

Some answers... 😄
(I'm glad to read your questions as they seem to imply that you're feeling better!)

why is value level superior? :)

IMHO it's easier to understand how the Swagger docs are rendered on value level. Probably, that's a matter of taste.

This change is a bit orthogonal to the rest: I could add data Swagger serviceName port again (I need some port to render the kubectl port-forward port:8080 command in the Swagger description. In theory it could be random, but I was unsure if a random number wouldn't lead to more confusion... 🤔 ).

so this PR undoes that solution?

Yes, no more prefixes as those aren't valid paths. (valid == A path that you may use to query a service.)

did the same limitation hold for the above solution /brig/i/status? if not why is this PR an improvement?

The prefixed paths were not executable at all. The improvement of this PR is that the rendered curl commands are executable (if you ran the rendered kubectl port-forward command before).

Our Servant servers seem to not handle CORS headers. So, we have three options here:

  • Teach them CORS
  • Setup some HTTP proxy (which knows CORS)
  • Document the limitation in the Swagger description at top level

I decided to apply the third option, because it's the simplest. (Finally, this in only about Swagger... 😉 )

why does legalhold has its own page?

Is that wrong? I just found this API and used it without really knowing anything about legalhold... 🙄

@battermann
Copy link
Contributor

Is that wrong? I just found this API and used it without really knowing anything about legalhold...

I'd say, it's wrong. legalhold is not a stand-alone service like brig, galley, etc.

Copy link
Contributor

@battermann battermann left a comment

Choose a reason for hiding this comment

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

LGTM

@supersven supersven force-pushed the sventennie/swagger-page-per-internal-endpoint branch from 2659bbf to d0cbb25 Compare February 24, 2023 14:55
@supersven supersven merged commit 1817eb8 into develop Feb 27, 2023
@supersven supersven deleted the sventennie/swagger-page-per-internal-endpoint branch February 27, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants