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

docs: ignore anchor for observability links #9412

Merged
merged 1 commit into from
May 28, 2024

Conversation

kkunapuli
Copy link
Contributor

@kkunapuli kkunapuli commented May 22, 2024

Ticket

RM-313

Description

Observability docs include 4 links that are valid, but are failing check-links. Add anchors to ignore list to reduce noise and ci failures.

Test Plan

None needed.

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@cla-bot cla-bot bot added the cla-signed label May 22, 2024
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@determined-ci determined-ci requested a review from a team May 22, 2024 18:42
@determined-ci determined-ci added the documentation Improvements or additions to documentation label May 22, 2024
@kkunapuli kkunapuli requested review from azhou-determined and removed request for a team May 22, 2024 18:42
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

Copy link

netlify bot commented May 22, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 18b6da4
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/664e3c7a4574d200087d09c3

"metrics",
"join-metrics",
"exposed-metrics",
"L139-L169",
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like sphinx finds github anchors problematic in general, let's just ignore a glob/regex for all github anchors? these individual line links are going to be especially hard to catch if we slightly change the link url.

there's linkcheck_anchors_ignore_for_url, https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-linkcheck_anchors_ignore_for_url which seems a good fit for our use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good idea, but I don't know that it's the right fit here.

linkcheck_anchors_ignore_for_url requires sphinx version 7.1+, but docs /requirements.txt pins sphinx to sphinx==4.2.0

I also think the brittleness of specifying specific anchors is a good thing in this case. Not all github links will necessarily be valid; shouldn't we check them one by one and manually "ok" them?

Copy link
Contributor

Choose a reason for hiding this comment

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

i see. then what you had before is fine.

@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

1 similar comment
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@kkunapuli kkunapuli merged commit 8a6f571 into main May 28, 2024
96 of 109 checks passed
@kkunapuli kkunapuli deleted the kunapuli/ignore-anchors-not-found branch May 28, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants