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

Support for redirects other than the old site ones. #156

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

Conversation

benjyw
Copy link
Sponsor Contributor

@benjyw benjyw commented Feb 17, 2024

Uses this to provide redirects required by
pantsbuild/pants#20544

@benjyw
Copy link
Sponsor Contributor Author

benjyw commented Feb 17, 2024

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 [
Copy link
Member

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?

Copy link
Sponsor Contributor Author

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.

Copy link
Member

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

Copy link
Sponsor Contributor Author

@benjyw benjyw Feb 17, 2024

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?

Copy link
Sponsor Contributor Author

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.

Copy link
Member

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

Copy link
Contributor

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 the 2.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.

Copy link
Sponsor Contributor Author

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.

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.

None yet

3 participants