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

Remove /docs/ from /topics/ URLs #90

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented Jun 25, 2024

We've decided earlier that we want short URLs on the form

https://valkey.io/topics/acl

The URLs (before this patch) contain /docs/topics/ and a trailing slash, like this:

https://valkey.io/docs/topics/acl/

This change is to make the URLs shorter and more close to what's been decided.

Additionally, in the docs repo, the links between the command pages and the topics pages are on the form "../topics/acl.md" and "../commands/client-kill.md" and this is also how the files are stored in the docs repo, so by putting "topics" at the same level as "commands" on the website too makes editing the docs more intuitive.

I can't get rid of the trailing slash right now, so with this patch, the URLs look like

https://valkey.io/topics/acl/

Redirects are added for the /docs/topics/ URLs.

We've decided earlier that we want short URLs on the form

    https://valkey.io/topics/acl

The URLs (before this patch) contain /docs/topics/ and a trailing slash,
like this:

    https://valkey.io/docs/topics/acl/

This is to make the URLs shorter and more close to what's been decided.

In the docs repo, the links between the command pages and the topics
pages are on the form "../topics/acl.md" and "../commands/client-kill.md"
so by putting "topics" at the same level as "commands" on the website
too makes editing the docs more intuitive.

I can't get rid of the trailing slash right now, so with this patch,
the URLs look like

    https://valkey.io/topics/acl/

Redirects are added for the /docs/topics/ URLs.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@stockholmux
Copy link
Member

Technical Evaluation

This PR doesn't seem to work for me. How was it tested?

  1. When I run init-topics.sh I get a page full of No such file or director errors; these errors are not present when using the main branch of the repo.
  2. I'm not sure this PR achieves what it says it achieves. The topics still reside at /docs/topics/foo/ but it breaks numerous things in the process. Looking at the output of zola build I see the following directory structure:
...
├── docs
│   ├── index.html
│   └── topics
│       ├── 2idx_0.png
│       ├── 2idx_1.png
│       ├── 2idx_2.png
│       ├── Connections_chart.png
│       ├── Data_size.png
│       ├── NUMA_chart.gif
│       ├── acl
│       │   └── index.html
│       ├── admin
│       │   └── index.html
│       ├── arm
│       │   └── index.html
│       ├── benchmark
│       │   └── index.html
│       ├── bitfields
│       │   └── index.html
...
  1. This is breaking the templating system. All the documentation topics yield the following:

Screenshot 2024-06-26 at 8 23 36 AM

Idea itself

I'm not sold that this is a good idea (in general). I have three concerns:

  1. The /docs/topics/ structure has existed since Valkey.io launch (April 10, to be exact). While there were only a handful of topics at the time these URLs have been in the wild for months. It looks like the PR attempts to have a mitigation with a redirect (aliases in the front matter), but general best practice is to treat URLs as immutable and limit linked redirects (for SEO).
  2. /docs/ was an intentional choice and what I thought the plan was all along. This effectively namespaces anything that needs to be pulled in from valkey-doc without fear of creating a collision with other items in the website repo. As an example: valkey-doc has directories called tools and modules - it is extremely likely that the website also would want to have a directory also called tools and modules that would have totally different content and different templates, so when we end up implementing either it would make it complex to resolve effectively.
  3. From my perspective, this is a big change that doesn't add meaningful functionality and impacts visitors. The PR that had the directory structure was posted on May 11 and didn't get merged until May 23 and the site wasn't pushed for a further 2 weeks. Any time before the site was published would have had near zero impact, but we're well past that now.

Perhaps a middle way would be to have a redirect for /topics/foo to /docs/topics/foo? This avoids the namespace issues, wouldn't impact on linking/SEO/templating, and would be a noop for 99.9% of visitors.

@zuiderkwast
Copy link
Contributor Author

I've tested it locally, run the init-topics script and tested both using zola serve and zola build. I may have forgotten to git add some file. You may have old generated files present. I'll look into it and come back on this.

The /docs/topics/ structure has existed since Valkey.io launch

From the start, this was done against the decision by TSC. We decided very early on that we want /topics/pagename. You ignored this. We talked about the lack of 301 redirects and concluded that it's good to get this right from the beginning. Still you ignored this and launched the pages with the wrong URLs.

2. /docs/ was an intentional choice and what I thought the plan was all along. This effectively namespaces anything that needs to be pulled in from valkey-doc

This is just not true. First, the command pages are not under docs. They're at /commands/name/.

Second, this was not decided by TSC. TSC on the other hand has decided that the URLs shall be /topics/pagename.

Third, the docs are the most important part of the website and deserve short URLs.

In my perspective, the docs are not yet really up. You get an unordered and incomplete list of pages with a disclaimer that the page may be deleted.

I'm willing to fight for the URLs and the docs. I'll maje sure we take a decision in the core team if necessary.

@zuiderkwast
Copy link
Contributor Author

I had missed to git add a file content/topics/_index.md. I added it now.

@zuiderkwast
Copy link
Contributor Author

3. (...) The PR that had the directory structure was posted

#67 didn't mention in the description that it changed the URL structure. I didn't expect that to change behind the scenes just because we're replacing the rendering engine, so I didn't detect this in the diff. I'd say you were hiding this deliberately.

@madolson
Copy link
Member

madolson commented Jun 26, 2024

/docs/ was an intentional choice and what I thought the plan was all along. This effectively namespaces anything that needs to be pulled in from valkey-doc without fear of creating a collision with other items in the website repo. As an example: valkey-doc has directories called tools and modules - it is extremely likely that the website also would want to have a directory also called tools and modules that would have totally different content and different templates, so when we end up implementing either it would make it complex to resolve effectively.

I thought we were going to keep just / for all the content. I think there might have just been a miss about that, I missed it in the review because it wasn't documented and I was just looking for dead links. Should have looked more closely.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@stockholmux
Copy link
Member

stockholmux commented Jun 28, 2024

Okay. Yesterday @madolson and @zuiderkwast did a sync over Zoom on this. Looks like there was a very early on miscommunication about the desired paths (as @madolson pointed out). Despite code reviews, the /docs/topics/ paths got approved and here we are.

Additionally, the other paths in valkey-io/valkey-docs aren't really as critical and (probably) won't take the form that they are today anyway (especially the... interesting... current structure of/clients/).

Moving on, let's transition the URLs to the forever home (/topics/). Also the missing file that @zuiderkwast added back in fixed the issues I was having on the PR in the original form.

Two things I would like:

  1. A double check with the on-page links in /topics/ work as well as the live site on valkey.io /docs/topics/ today. I've done a cursory check and it seems fine, but it's moving a ton of pages, so it's worth checking links to make sure there isn't a corner case in the regex where we create broken links.
  2. If someone is setup today with the local environment at /docs/topics/ and re-runs init-topics.sh to get the new paths, they'll run into a nasty, full page error and Zola won't build or server the site:
Error: Failed to serve the site
Error: Found path collisions:
....

This error is due to the redirect files overlapping with the old content files. It might be worth adding a path check to init-topics.sh to see if /content/docs/topics exists and erroring the bash script out on that condition. I'm glad to add this bit of logic, but I won't be able to attend to it for several days.

@zuiderkwast
Copy link
Contributor Author

  • A double check with the on-page links in /topics/ work as well as the live site on valkey.io /docs/topics/ today. I've done a cursory check and it seems fine, but it's moving a ton of pages, so it's worth checking links to make sure there isn't a corner case in the regex where we create broken links.

Sure, Madelyn promised to help with this. Or else, I'll do it. Anyway, we won't merge this until it's done.

  • If someone is setup today with the local environment at /docs/topics/ and re-runs init-topics.sh to get the new paths, they'll run into a nasty, full page error and Zola won't build or server the site:

Good point! Would it be fine if the init-topics.sh script would fix this automatically (deletes the old stub files) or do you prefer that it errors out?

@stockholmux
Copy link
Member

@zuiderkwast Sounds good.

Maybe do a check something like this at the top of init-topics.sh

# check for old style /docs/topics
if [ -d "content/docs/topics" ]; then
    echo "Documentation topic source files have moved. Remove /content/docs/topics before proceeding."
    exit 1
fi

Additionally, silence the error from ln when trying to overwrite the symlink
'build-topics' with an identical symlink to the same target.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Copy link
Member

@stockholmux stockholmux left a comment

Choose a reason for hiding this comment

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

I went through and looked at every link. Everything seems to perform as well as the live site. I say we go ahead.

(I did see some unrelated link problems, but I'll make an issue for those elsewhere)

@zuiderkwast
Copy link
Contributor Author

Awesome! Thanks!

@zuiderkwast zuiderkwast merged commit ee0e458 into valkey-io:main Jul 4, 2024
1 check passed
@zuiderkwast zuiderkwast deleted the remove-docs-url-component branch July 4, 2024 22:11
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