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.

Sign up for GitHub

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

Jump to bottom

Forbid full internal links #14016

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

Forbid full internal links #14016

wants to merge 2 commits into from

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Feb 8, 2025 •
edited
Loading

This PR is best reviewed commit by commit:

  • fc3670c adds a lint that forbids internal links (links that start with https://www.pulumi.com/{registry,docs}/).
  • c67286b fixes up lint violations with a shell script. The commit message itself describes the shell script used.

@iwahbe iwahbe requested a review from a team as a code owner February 8, 2025 23:22
@iwahbe iwahbe self-assigned this Feb 8, 2025
@iwahbe iwahbe force-pushed the iwahbe/update-linter branch 3 times, most recently from d4d1e4f to 89a6d85 Compare February 11, 2025 10:23
Base automatically changed from iwahbe/update-linter to master February 12, 2025 13:01
@iwahbe iwahbe force-pushed the iwahbe/forbid-full-internal-links branch from c67286b to 265e0e4 Compare February 12, 2025 13:01
@iwahbe iwahbe force-pushed the iwahbe/forbid-full-internal-links branch from 265e0e4 to de7e9d4 Compare February 12, 2025 13:25
@iwahbe
Add a linter which forbids full links to pulumi.com/{registry,docs}
0ec68d1 
This is motivated by #14006 (comment).
@iwahbe
Apply a programatic fix to the lint
8dbf582 
The lint can be fixed cleanly with:

```sh
make lint | rg '/Users/.*.md' -o | xargs gsed -i 's#](https://www.pulumi.com/registry#](/registry#g; s#](https://www.pulumi.com/docs#](/docs#g'
```

It walks `sed` over every file that fails the lint and applies two corrections:

- `](https://www.pulumi.com/registry` -> `/registry`
- `](https://www.pulumi.com/docs` -> `](/docs`

The leading `](` ensures that the text is part of a markdown link.
@iwahbe iwahbe force-pushed the iwahbe/forbid-full-internal-links branch from de7e9d4 to 8dbf582 Compare February 12, 2025 13:26
@pulumi-bot

This comment was marked as outdated.

Sign in to view
@pulumi-bot
Copy link
Collaborator

Your site preview for commit 8dbf582 is ready! 🎉

http://www-testing-pulumi-docs-origin-pr-14016-8dbf582c.s3-website.us-west-2.amazonaws.com.

@iwahbe iwahbe requested a review from interurban February 12, 2025 17:42
thoward
thoward approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@thoward thoward left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall, the change looks good. Thanks for adding a lint rule! I'm concerned there might be valid places where relative links don't work and the fully qualified url might be necessary.

I'd like to see every changed link tested to ensure we aren't introducing new broken links that are invisible to our automation/relative linking strategy. For that, I'd suggest a one-time manual check of each link changed as part of this PR, to make sure they are still working. I rough glance suggests this is about 50 links to check. Tedious, but not too bad.

@iwahbe
Copy link
Member Author

iwahbe commented Feb 12, 2025

Overall, the change looks good. Thanks for adding a lint rule! I'm concerned there might be valid places where relative links don't work and the fully qualified url might be necessary.

I'd be very surprised if this was the case, but it's theoretically possible. I believe that interpreting /<path> as <baseurl>/<path> is a property of Hugo, and should work for all markdown links.

I'd like to see every changed link tested to ensure we aren't introducing new broken links that are invisible to our automation/relative linking strategy.

We already have automation that monitors broken links in the #docs-ops channel. Can we instead monitor that? If post-merge the number of broken links increases we can investigate further or revert.

For that, I'd suggest a one-time manual check of each link changed as part of this PR, to make sure they are still working. I rough glance suggests this is about 50 links to check. Tedious, but not too bad.

The diff for 8dbf582 is +509 -484, and every - represents a change in at least one URL. I'd guess there are ~600 URLs changed here, but there are at least 484 URLs changed. I don't think a manual review is feasible here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thoward thoward thoward approved these changes

@interurban interurban Awaiting requested review from interurban

Assignees

@iwahbe iwahbe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iwahbe @pulumi-bot @thoward