openzipkin / zipkin-api

Zipkin's language independent model and HTTP Api Definitions
https://zipkin.io/zipkin-api/
Apache License 2.0
59 stars 32 forks source link

Update swagger ui to latest version and use vite for bundling. #100

Closed SamTV12345 closed 7 months ago

SamTV12345 commented 7 months ago

This removes the copied swagger ui js and css artifacts and instead uses vite to bundle our application. That way we won't get any conflicts.

Be aware that you need to change the Pages deployment from /root to /dist after merging this pr.

Closes #98

codefromthecrypt commented 7 months ago

curious.. why can't we just flatten dist to root like before?

SamTV12345 commented 7 months ago

curious.. why can't we just flatten dist to root like before?

This is because vite requires an own index.html etc. If we flatten out at root we would clutter the other necessary files like vite.config.ts. Like this we have a separation of concerns. Dist contains the bundled artifacts and everything else is required for developing.

codefromthecrypt commented 7 months ago

This is because vite requires an own index.html etc. If we flatten out at root we would clutter the other necessary files like vite.config.ts. Like this we have a separation of concerns. Dist contains the bundled artifacts and everything else is required for developing.

The thing is that we aren't "developing" swagger though, we are just consuming it with a small modification to index.html. If we are "developing" I don't think this is the right approach, and it is also overshooting the requirement by a large margin.

What I asked for help on was a lot simpler, and won't add any new development requirements to the people who are already not developing ;) https://github.com/openzipkin/zipkin-api/pull/101

So, swagger UI is just something we consume. let's KISS (keep it simple and stupid). By unzipping we also don't break things that update this.

The master branch otoh is more like development as it actually has CI, etc. If you'd like to go to town on that and update it to pnpm please do, but make sure you keep the maven build working because the protos are published that way.

codefromthecrypt commented 7 months ago

ps for longer in case curious on " I don't think this is the right approach," what I mean is that developing off gh-pages is a very bad experience. basically we would want to change master to produce this content and then have some automation to auto-publish the gh-pages branch with the result of that. history tells me each time we try something like this there are some glitches to work out. also, we'd have to want to take on responsibility of the dev side of swagger.. so we'd want to hav a good reason to do that. if all we are doing is making the same dist as they publish, it is kindof doing a lot of work for value already produced. hope the explanation helps and sorry if I misled you down this path!

SamTV12345 commented 7 months ago

No worries. Thanks for the explanation. I'm always a bit scared when checking in source code from other vendors. That's why I thought it would be worthwhile to add this to the gh-pages branch. Ideally we would put that in master and change the deployment to GitHub Actions. That way we don't have any artifacts in the repo.

codefromthecrypt commented 7 months ago

@SamTV12345 the approach you mention is better for sure. How about if you clean up master, and if you feel the same way, give a try there with the build and push to gh-pages via some GitHub action. basically it is the same as any static app like Netlify just we are doing the push manually. As long as the build runs on master (not here), and we can keep the deployment root of gh-pages as-is.. I'm happy!

codefromthecrypt commented 7 months ago

updating didn't work, so yeah now like it or not, if we want to crush the XSS we have to do something more involved. Made some notes. I'm not sure you can change the base of this PR, but anyway turns out you were on the right path as it wasn't as simple as updating swagger to squash https://github.com/openzipkin/zipkin-api/issues/98#issuecomment-1953255705