-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add configurable redirect #142
Conversation
0ece497
to
6cab40f
Compare
6cab40f
to
7dfd4c7
Compare
@alanshaw @olizilla per what we talked about earlier today, just made sure and in grafana we track status code https://daghouse.grafana.net/d/PCqBFbzVz/w3link?orgId=1 . So, we will already be able to see the amount of traffic that is not served by Cache + Freeway and is redirected. So, we don't really need any action in this PR for metrics :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM, but I think the status code needs to change...
packages/edge-gateway/src/gateway.js
Outdated
`https://${cid}.ipfs.${env.ipfsGatewayRedirectHostname}${pathname}${search}` | ||
) | ||
|
||
return Response.redirect(url.toString(), 303) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[303] indicates that the redirects don't link to the requested resource itself, but to another page (such as a confirmation page, a representation of a real-world object — see HTTP range-14 — or an upload-progress page). This response code is often sent back as a result of PUT or POST. The method used to display this redirected page is always GET.
Maybe we want https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olizilla 's proposal was 303 https://hackmd.io/@olizilla/w3s-redirect . So, would like to get input if there was something that made 303 the suggestion.
For the documentation, I agree that 307 seems more appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, agree, 307 sounds closer to what we want. I didn't realise that "303 See other" implied we were linking to a different resource.
🤖 I have created a release *beep* *boop* --- ## [1.10.0](edge-gateway-v1.9.3...edge-gateway-v1.10.0) (2023-03-07) ### Features * add configurable redirect ([#142](#142)) ([cfb8798](cfb8798)), closes [#141](#141) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adds configurable redirect via a secret that can easily be deployed and removed if needed. Later on, we can decide to remove the race code
Also made github actions use Node 16 because apparently it started installing Node18 now and we require Node16 https://github.com/web3-storage/reads/blob/main/package.json#L46 . In follow up PR I will also update to Node18
Closes #141