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

check-markdown-links is painfully flaky #4575

Closed
alice-i-cecile opened this issue Apr 24, 2022 · 6 comments
Closed

check-markdown-links is painfully flaky #4575

alice-i-cecile opened this issue Apr 24, 2022 · 6 comments
Labels
A-Build-System Related to build systems or continuous integration A-Meta About the project itself

Comments

@alice-i-cecile
Copy link
Member

This step of the CI is incredibly flaky (see #4469 for an example). Blocking merges is pointless friction, and confuses new contributors.

There are several options here:

  1. Make sure the check doesn't block CI.
  2. Use an alternative strategy for testing for dead links that is more reliable.
  3. Somehow scope the check to only make sure it checks newly added or modified links.
  4. Move dead linking to an action that is run once a day, with some form of alerting.

These are not mutually exclusive by any means. @mockersf, do you have preferences?
@alice-i-cecile alice-i-cecile added A-Build-System Related to build systems or continuous integration A-Meta About the project itself labels Apr 24, 2022
@superdump
Copy link
Contributor

Do we have any idea why it fails? What errors are returned when it does fail? All I saw seemed to be a github token error message.

@superdump
Copy link
Contributor

The errors in #4469 seem to be that requests to https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews get a 403 response, meaning the requests are forbidden, usually an authorisation problem, but I wonder if it’s rate limiting or something.

@Nilirad
Copy link
Contributor

Nilirad commented Apr 24, 2022

I think internal links should always be valid, so it is good to check them on the PR.

On line other hand, external links are outside of our control, and therefore it makes more sense to check them outside of the PR workflow, e.g. periodically.

@mockersf mockersf mentioned this issue Apr 24, 2022
Closed
@mockersf
Copy link
Member

The recent failures are all (all the ones I checked at least) on a link to docs.github.com. My guess is that GitHub added a protection to avoid being ddos from actions. Links to GitHub.com were already ignored, I added docs.github.com to the ignore list in #4578

bors bot pushed a commit that referenced this issue Apr 24, 2022
@mockersf
do not check links on docs.github.com (#4578)
dd57a94 
# Objective

- related to #4575, but not a complete fix
- links to GitHub.com can't be checked from inside a GitHub Actions as GitHub is protecting itself from being flooded by an action execution
- it seems they added that protection to GitHub doc site

## Solution

- Ignore links to docs.github.com
@rparrett
Copy link
Contributor

rparrett commented May 6, 2022

I was poking around at this and discovered lychee-action as an alternative to github-action-markdown-link-check.

It's a rust project that seems to have all the features that we're using from github-action-markdown-link-check but also supports using a personal access token for requests to github as a way of getting around rate limits.

A personal token with no extra permissions is enough to be able to check public repos links.

@alice-i-cecile
Copy link
Member Author

I'd be interested in trying that out. Want to make a PR to replace it?

@alice-i-cecile alice-i-cecile mentioned this issue May 17, 2022
Closed
exjam pushed a commit to exjam/bevy that referenced this issue May 22, 2022
@mockersf @exjam
do not check links on docs.github.com (bevyengine#4578)
8912243 
# Objective

- related to bevyengine#4575, but not a complete fix
- links to GitHub.com can't be checked from inside a GitHub Actions as GitHub is protecting itself from being flooded by an action execution
- it seems they added that protection to GitHub doc site

## Solution

- Ignore links to docs.github.com
@alice-i-cecile alice-i-cecile mentioned this issue May 25, 2022
Closed
@bors bors bot closed this as completed in 5dd30b6 May 25, 2022
james7132 pushed a commit to james7132/bevy that referenced this issue Jun 7, 2022
@cart @james7132
Remove markdown dead link check (bevyengine#4839)
5d49162 
# Objective

This fails constantly and causes more pain than it is worth.

## Solution

Remove dead link checks.

Alternative to bevyengine#4837, which is more granular but ironically still fails to build. I'm in favor of the nuclear option.

Fixes bevyengine#4575
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@mockersf @ItsDoot
do not check links on docs.github.com (bevyengine#4578)
38d827b 
# Objective

- related to bevyengine#4575, but not a complete fix
- links to GitHub.com can't be checked from inside a GitHub Actions as GitHub is protecting itself from being flooded by an action execution
- it seems they added that protection to GitHub doc site

## Solution

- Ignore links to docs.github.com
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@cart @ItsDoot
Remove markdown dead link check (bevyengine#4839)
1224a07 
# Objective

This fails constantly and causes more pain than it is worth.

## Solution

Remove dead link checks.

Alternative to bevyengine#4837, which is more granular but ironically still fails to build. I'm in favor of the nuclear option.

Fixes bevyengine#4575
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Build-System Related to build systems or continuous integration A-Meta About the project itself
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - Remove markdown dead link check cart/bevy
5 participants
@rparrett @superdump @alice-i-cecile @mockersf @Nilirad