-
Notifications
You must be signed in to change notification settings - Fork 18
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
Support for redirects other than the old site ones. #156
base: main
Are you sure you want to change the base?
Conversation
Uses this to provide redirects required by pantsbuild/pants#20544
Note that CI will only pass, and so this can only be merged, after pantsbuild/pants#20544 is merged and copied over, which only happens when we publish. So we'll need to figure some way to then publish again, to get the redirects in. In this specific case we can also not cherrypick to 2.19, but since adding redirects seems like an important capability to have, let's think of a good solution here. |
redirects.js
Outdated
@@ -0,0 +1,10 @@ | |||
export default [ |
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.
Can we rename this to make it more obvious what kinds of redirects these are? Something like renamed_path_redirects
?
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.
Sure, but I'm also concerned about the larger question of the fact that CI can't pass on this until the new pages exist in the repo, which requires publishing, and then re-publishing so the redirects go live.
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.
I think that's a good thing. It's catching bad redirects and won't publish.
I think the correct thing to do is to add the infra in this PR, and then update it in the docs sync PR when we sync renamed paths
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.
How would that work? Isn't that an automatic PR during release?
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.
It is a good thing, but it means that there is an impedance mismatch between how docs publishing works and implementing redirects.
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.
Yeah, whoever would've been reviewing that PR would have to do it.
We could move that file into the pants repo and special-case "sync"-ing it.
But then, we might wanna invest in automation on making sure the pants repo "builds" the docs healthily
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.
I think having this file in the main repo and syncing it naively/wholesale would be hard to manage due to cherry-picks. For instance, we're adding a redirect here for 2.19 only:
- If we put the redirect file update into the
main
PR, then a release/doc sync from main won't work if that happens before the2.19.x
changes are in - If we put it into the 2.19 cherry pick PR, it will be overridden by the next main sync.
(I imagine we can make the automation more intelligent to solve this, but that means more complexity too.)
I wonder if we should have live chat about our approach to documentation and releases so we can get to shared direction more quickly rather than trying to hash it out over text.
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.
Yeah I was pondering this, and I think redirects are fundamentally not a versioned thing.
Uses this to provide redirects required by
pantsbuild/pants#20544