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

fix: redirect static-router/doc/master to stable docs (#568) #571

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

softmoth
Copy link

@softmoth softmoth commented Sep 21, 2024

Testing

I didn't test this, I'm not sure how to do so.

Rationale for stable

I decided that a redirect to //doc.rust-lang.org/stable/* makes the most sense. While the doc-router redirects /master to /nightly, that is a mistake that could be happening with current users (e.g., someone is looking at stable docs and wants the latest, so they just change the URL in the browser to master).

This static-router redirect should really only be hit when coming from an old forum post or similar (it was Rust 0.17 docs, after all). Since stable is the default for any miscellaneous path on doc-router, it seems like the appropriate redirect target here.

Redirecting to doc.rust-lang.org

I decided to redirect to //doc.rust-lang.org instead of stay on //static.rust-lang.org, because there could be some helpful error handling or other redirects in doc-router that we'd want to take advantage of. Also, it is the preferred entry point for all documentation, so it's best to keep static as an implementation detail when possible.

@MarcoIeni
Copy link
Member

MarcoIeni commented Sep 23, 2024

I pushed a fix (the second argument of the redirect function was `body) and I applied the changes in the staging environment.

https://cloudfront-dev-static.rust-lang.org/doc/master/rust.html works!
reference.html is not working yet because of caching on my browser probably.

index.js is for cloudfront. We also need to update fastly. I forgot to mention this in the issue, sorry!

Here's the location:

https://github.com/softmoth/simpleinfra/blob/8df16d45f2b95e77081928ea9f66f53267b91e80/terragrunt/modules/release-distribution/fastly-static.tf#L97

Do you want to look into that as well?

@softmoth
Copy link
Author

Thank you for fixing the body bug. My attention span is that of a gnat, apparently.

I've made a stab at the fastly VCL change. I didn't test it either, but hopefully it's correct.

@MarcoIeni
Copy link
Member

My attention span is that of a gnat, apparently.

We should try to add a linter for that code honestly 😅

Thanks, I'll test this soon!

Copy link
Member

@jdno jdno left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. If you need any help with Fastly, feel free to ping me. 🙂

snippet {
# This was an abandoned copy of Rust 1.17.0-era docs; redirect to current docs
name = "redirect /doc/master to /stable"
type = "error"
Copy link
Member

Choose a reason for hiding this comment

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

The snippet will not be executed, since we never make it to the error state in the VCL lifecycle. That's why the existing snippet that redirects rustup.sh first has a snippet that detects requests to that endpoint and then fails them with a custom error that we're handling manually.

snippet {
  name    = "detect rustup.sh requests"
  type    = "recv"
  content = <<-VCL
    if (req.url ~ "^\/rustup\.sh$") {
      error 618 "redirect";
    }
  VCL
}

We need something like this for the docs as well. I suggest that we use a new error code (e.g. 619), and then replicate how rustup.sh is handled.

name = "redirect /doc/master to /stable"
type = "error"
content = <<-VCL
if (req.url ~ "^\/doc/master(\/|$)")?$") {
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not counting wrong, there's a ( missing or a ) too many somewhere. 😅

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.

3 participants