Links replace

[aleks-stefanovic]

No description provided.

[k8s-ci-robot]

Welcome @alex-akv!

It looks like this is your first PR to kubernetes-sigs/agent-sandbox 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/agent-sandbox has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @alex-akv. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[netlify]

✅ Deploy Preview for agent-sandbox ready!

Name	Link
🔨 Latest commit	af77d87
🔍 Latest deploy log	https://app.netlify.com/projects/agent-sandbox/deploys/69698bc9227b440007b1251c
😎 Deploy Preview	https://deploy-preview-186--agent-sandbox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[janetkuo]

Ideally it should link to the doc in the same commit (instead of main). Would you explain why this change is needed?

[jkallogjeri]

This change is required because the link as is currently does not work on the website (https://agent-sandbox.sigs.k8s.io/docs/guides/jupyterlab/), by changing it to an actual link it ensures that the website sends you to the github link instead of https://agent-sandbox.sigs.k8s.io/docs/INSTALL.md (which it does now), this issue persists everywhere we have this.

[janetkuo]

/ok-to-test

[barney-s]

Thanks for fixing the broken links on the documentation site. I understand this change is to make the links work on agent-sandbox.sigs.k8s.io.

However, hardcoding links to the main branch will cause the documentation to be misleading for users browsing older tagged versions. The best long-term solution is to configure the Hugo static site generator to correctly resolve these relative links. For now, could we explore a relative link format that is compatible with Hugo? If not, we should create a follow-up issue to address the documentation build process.

[barney-s]

While this fixes the link on the rendered website, it makes the documentation less maintainable and potentially misleading for users on different versions. Pinning the link to the main branch means that anyone viewing this document from a tagged release will not see the corresponding version of the installation guide. We should investigate a solution within our static site generator to resolve these links correctly without hardcoding the branch.

[aleks-stefanovic]

@barney-s, I think I have a solution for this and would like to hear your opinion on it. The solution could be to create a new layout for instance layouts/_default/_markup/render-link.html with contents:

{{- $url := .Destination -}}
{{- $text := .Text -}}
{{- $title := .Title -}}

{{- /* Only rewrite README.md links */ -}}
{{- if or (strings.Contains $url "README.md") (strings.HasSuffix $url "README.md") -}}
  {{- /* ../../README.md -> /docs/readme/ (root README) */ -}}
  {{- if hasPrefix $url "../../README.md" -}}
    {{- $url = "/docs/readme/" -}}
    {{- if strings.Contains .Destination "#" -}}
      {{- $anchor := index (split .Destination "#") 1 -}}
      {{- $url = printf "/docs/readme/#%s" (lower $anchor) -}}
    {{- end -}}
  {{- /* ./README.md or README.md -> same directory readme */ -}}
  {{- else if or (hasPrefix $url "./README.md") (eq $url "README.md") -}}
    {{- /* Get parent directory from current page path */ -}}
    {{- $pagePath := .Page.File.Dir -}}
    {{- $url = printf "/docs/%sreadme/" $pagePath -}}
  {{- end -}}
{{- end -}}

<a href="{{ $url | safeURL }}"{{- with $title }} title="{{ . }}"{{ end -}}>{{ $text | safeHTML }}</a>

Where we would keep relative paths as they are, and during hugo build time, the render hook intercepts those links and rewrites them to correct site URLs. I have tested this and it worked successfully.

Some of the tradeoffs would include a requirement for updating the render hook template if hugo URL structure changes.

[barney-s]

SGTM lets do this. If it breaks for old versions we can revisit.

[janetkuo]

@barney-s I saw you lgtm'd, but this change hasn't been applied yet

[aleks-stefanovic]

@barney-s, @janetkuo I have pushed this approved change in commit 43f3b4d

Please approve the change.

[janetkuo]

With this new layout change, the changes to md files must be reverted to use relative paths

[aleks-stefanovic]

@janetkuo Reverted the links back in the last commit.

[barney-s]

/lgtm

/approve ready @justinsb @janetkuo

[janetkuo]

With this new layout change, the changes to md files must be reverted to use relative paths

[aleks-stefanovic]

With this new layout change, the changes to md files must be reverted to use relative paths

Right! I have reverted the links back in the last commit.

[barney-s]

The approach to use a render hook for handling relative links is good. However, I have some concerns about the constructed URLs in the hook implementation and the consistency of changes in the example files.

[barney-s]

This change replaces a relative link with an absolute one. Since the goal of this PR (via the render hook) is to support relative links effectively, shouldn't this remain as ../../README.md? The render hook seems designed to handle exactly this case.

[janetkuo]

+1 this needs to be reverted as well

[aleks-stefanovic]

Reverted in commit: af77d87

[barney-s]

If .Page.File.Dir returns the path relative to the content directory (e.g., docs/guides/jupyterlab/), prepending /docs/ here might result in a duplicated prefix (e.g., /docs/docs/guides/...). Please verify the value of $pagePath in your environment.

[barney-s]

Appending readme/ assumes that the referenced README.md is rendered as a page named readme. However, in many cases (like jupyterlab), the README.md content might be served via _index.md (the section index). If so, this link might point to a non-existent page.

[barney-s]

Similarly, this rewrites ../../README.md to /docs/readme/. Please ensure that the root README.md is indeed served at this specific path and not at /docs/ or another location.

[barney-s]

Using lower is a reasonable default for anchors, but Hugo's ID generation can be complex (e.g., handling spaces, special characters). Ensure this transformation matches the actual IDs generated in the target documents.

[aleks-stefanovic]

@barney-s After further investigating and testing based on your latest comments I discovered the core issue was that the markdownify function creates a new rendering context where .Page variables don't reflect the actual page location. Given that I would like to hear your opinion on another approach which is to preprocess markdown context in the include-file shortcode to rewrite relative README/INSTALL links to absolute hugo urls before calling markdownify. For example:

include-file.html would look like:

{{- $file := (.Get "file") -}}
{{- $md := resources.Get $file -}}

{{/* Remove first heading */}}
{{- $content := replaceRE `(\s*#[^\n$]*\n*)` "" $md.Content 1 -}}

{{/* Get the current page's section path */}}
{{- $pagePath := .Page.RelPermalink -}}
{{- $sectionPath := replaceRE "/[^/]+/$" "/" $pagePath -}}

{{/* Fix all relative README.md and INSTALL.md links */}}
{{- $content = replaceRE `\]\(\.\/README\.md(#[^)]+)?\)` (printf "](%s$1)" (strings.TrimSuffix "/" $sectionPath)) $content -}}
{{- $content = replaceRE `\]\(\.\.\/\.\./(README|INSTALL)\.md(#[^)]+)?\)` "](/docs/getting_started/$2)" $content -}}

{{- $content | markdownify -}}

And in this case, the render-links.html script is not needed.

The above approach is tested and the result was a success.

[janetkuo]

@alex-akv the include-file.html approach sounds good.

Regarding the regex, \]\(\.\/README\.md(#[^)]+)?\) requires the ./ prefix, which means that it will miss links like [Text](README.md), so please adjust the regex to optionalize the ./: \]\((\./)?README\.md(#[^)]+)?\)

[aleks-stefanovic]

@alex-akv the include-file.html approach sounds good.

Regarding the regex, \]\(\.\/README\.md(#[^)]+)?\) requires the ./ prefix, which means that it will miss links like [Text](README.md), so please adjust the regex to optionalize the ./: \]\((\./)?README\.md(#[^)]+)?\)

@janetkuo I have pushed the new approach with adjusted regex in commit: af77d87

[janetkuo]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alex-akv, barney-s, janetkuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• examples/OWNERS [barney-s,janetkuo]
• site/OWNERS [barney-s,janetkuo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment