search

hypertrace / hypertrace-service

Multiple hypertrace services combined together to form a single service.
Other
4 stars 15 forks source link

fix: rewrite ui urls #37

Closed aaron-steinfeld closed 4 years ago

aaron-steinfeld commented 4 years ago

This resolves https://github.com/hypertrace/hypertrace/issues/71 The problem with #36, the original approach is that it serves the page by overloading the 404 response. While this is functionally correct, it's semantically incorrect because the response is still technically a 404 error and shows up as such: image

jcchavezs commented 4 years ago

I would not merge this PR into main but instead I would do it on top of @JBAhire's work who is adding these checks on CI.

aaron-steinfeld commented 4 years ago

I would not merge this PR into main but instead I would do it on top of @JBAhire's work who is adding these checks on CI.

Pointer to where that is? I'm not sure we should wait to fix the outstanding issue for e2e tests to come in. Also while not opposed to a test, of all of the things that are likely to break/worth e2e testing, that this would be fairly low on my list. The server config is not something that changes is frequently or is particularly brittle.

jcchavezs commented 4 years ago

I am sorry I misread this repo as hypertrace/hypertrace. I still think tho we should somehow make sure this does not happen with a sort of dumb E2E make sure this urls are still available. How complex to spin up the service and call this URLs to make sure they don't return a 404 they are.

While I think this might fix the problem I can't test it right now and we don't have any sort of automated test for this yet hence don't feel like merging it myself. Tomorrow I will try it and if that is alright I will merge it.

kotharironak commented 4 years ago

Tried locally, works fine.

JBAhire commented 4 years ago

This also passes deep-link checks in CI so yeah we are good!