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

[Merged by Bors] - GitHub Action: Check local Markdown links on push #2067

Closed
wants to merge 32 commits into from

Conversation

FlyingRatBull
Copy link
Contributor

@FlyingRatBull FlyingRatBull commented May 1, 2021

Adds an GitHub Action to check all local (non http://, https:// ) links in all Markdown files of the repository for liveness.
Fails if a file is not found.

Goal

This should help maintaining the quality of the documentation.

Impact

Takes ~24 seconds currently and found 3 dead links (pull requests already created).

Dependent PRs

Info

See markdown-link-check.

Example output

FILE: ./docs/profiling.md

1 links checked.

FILE: ./docs/plugins_guidelines.md

37 links checked.

FILE: ./docs/linters.md
[✖] ../.github/linters/markdown-lint.yml → Status: 400 [Error: ENOENT: no such file or directory, access '/github/workspace/.github/linters/markdown-lint.yml'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'access',
  path: '/github/workspace/.github/linters/markdown-lint.yml'
}

Improvements

  • Can also be used to check external links, but fails because of:
    • Too many requests (429) responses:
FILE: ./CHANGELOG.md
[✖] https://github.com/bevyengine/bevy/pull/1762 → Status: 429
  • crates.io links respond 404
FILE: ./README.md
[✖] https://crates.io/crates/bevy → Status: 404

.github/workflows/action.yml Outdated Show resolved Hide resolved
@mockersf
Copy link
Member

mockersf commented May 1, 2021

Could you put this action into our existing CI workflow? https://github.com/bevyengine/bevy/blob/main/.github/workflows/ci.yml
And a check for it by job name in Bors config: https://github.com/bevyengine/bevy/blob/main/.github/bors.toml

For external links

@mockersf mockersf added the A-Build-System Related to build systems or continuous integration label May 1, 2021
* Fix [404 on crates.io](rust-lang/crates.io#788)
* Only block external checks on github.com
@FlyingRatBull
Copy link
Contributor Author

I added external links to be checked, but for whatever reason, it gets stuck in a redirect loop for https://developer.android.com/distribute/best-practices/develop/target-sdk, which I cannot reproduce with curl/firefox:

[✖] https://developer.android.com/distribute/best-practices/develop/target-sdk → Status: 0 Error: Exceeded maxRedirects. Probably stuck in a redirect loop https://developer.android.com/distribute/best-practices/develop/target-sdk
    at Redirect.onResponse (/usr/local/lib/node_modules/markdown-link-check/node_modules/request/lib/redirect.js:98:27)
    at Request.onRequestResponse (/usr/local/lib/node_modules/markdown-link-check/node_modules/request/request.js:986:22)
    at ClientRequest.emit (node:events:365:28)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (node:_http_client:621:27)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:127:17)
    at TLSSocket.socketOnData (node:_http_client:487:22)
    at TLSSocket.emit (node:events:365:28)
    at addChunk (node:internal/streams/readable:314:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at TLSSocket.Readable.push (node:internal/streams/readable:228:10)

Should I explicitly ignore this link in the markdown, in the configuration file or what would be the desired action for this?

I'll move the action into the ci.yml soon.

@FlyingRatBull
Copy link
Contributor Author

I moved the action into ci.yml and referenced it in bors.toml.
The total runtime now is ~34 seconds and besides the links in the mentioned PRs and the on commented above, it all runs well.

@mockersf
Copy link
Member

mockersf commented May 1, 2021

Should I explicitly ignore this link in the markdown, in the configuration file or what would be the desired action for this?

Not sure why it would fail, it's probably fine to ignore it unless someone else has an idea?

Small nit: I think the config file should go in https://github.com/bevyengine/bevy/tree/main/.github/linters. What do you think?

@FlyingRatBull
Copy link
Contributor Author

Not sure why it would fail, it's probably fine to ignore it unless someone else has an idea?

I ignored it in the Markdown file as opposed to the configuration file because this way, if the link get's removed/changed, the ignore block can be reviewed as well.

Small nit: I think the config file should go in https://github.com/bevyengine/bevy/tree/main/.github/linters. What do you think?

Good idea. Moved it there and renamed it to markdown-link-check.json as well.

@FlyingRatBull
Copy link
Contributor Author

Can bors be triggered again? The new check should now succeed, now that the dependent PRs have been merged.

@mockersf
Copy link
Member

mockersf commented May 1, 2021

you'll need to rebase your PR on main to fix the CI

@FlyingRatBull
Copy link
Contributor Author

you'll need to rebase your PR on main to fix the CI

Thanks, will do

@FlyingRatBull
Copy link
Contributor Author

Sorry for the messy git - There was some confusion because of a duplicate broken link.

@cart
Copy link
Member

cart commented May 2, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 2, 2021
Adds an GitHub Action to check all local (non http://, https:// ) links in all Markdown files of the repository for liveness.
Fails if a file is not found.

# Goal
This should help maintaining the quality of the documentation.

# Impact
Takes ~24 seconds currently and found 3 dead links (pull requests already created).

# Dependent PRs
* #2064 
* #2065 
* #2066

# Info
See [markdown-link-check](https://github.com/marketplace/actions/markdown-link-check).

# Example output
```
FILE: ./docs/profiling.md

1 links checked.

FILE: ./docs/plugins_guidelines.md

37 links checked.

FILE: ./docs/linters.md
[✖] ../.github/linters/markdown-lint.yml → Status: 400 [Error: ENOENT: no such file or directory, access '/github/workspace/.github/linters/markdown-lint.yml'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'access',
  path: '/github/workspace/.github/linters/markdown-lint.yml'
}
```

# Improvements
* Can also be used to check external links, but fails because of:
  * Too many requests (429) responses:
```
FILE: ./CHANGELOG.md
[✖] #1762 → Status: 429
```
   * crates.io links respond 404
```
FILE: ./README.md
[✖] https://crates.io/crates/bevy → Status: 404
```
@bors bors bot changed the title GitHub Action: Check local Markdown links on push [Merged by Bors] - GitHub Action: Check local Markdown links on push May 2, 2021
@bors bors bot closed this May 2, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
Adds an GitHub Action to check all local (non http://, https:// ) links in all Markdown files of the repository for liveness.
Fails if a file is not found.

# Goal
This should help maintaining the quality of the documentation.

# Impact
Takes ~24 seconds currently and found 3 dead links (pull requests already created).

# Dependent PRs
* bevyengine#2064 
* bevyengine#2065 
* bevyengine#2066

# Info
See [markdown-link-check](https://github.com/marketplace/actions/markdown-link-check).

# Example output
```
FILE: ./docs/profiling.md

1 links checked.

FILE: ./docs/plugins_guidelines.md

37 links checked.

FILE: ./docs/linters.md
[✖] ../.github/linters/markdown-lint.yml → Status: 400 [Error: ENOENT: no such file or directory, access '/github/workspace/.github/linters/markdown-lint.yml'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'access',
  path: '/github/workspace/.github/linters/markdown-lint.yml'
}
```

# Improvements
* Can also be used to check external links, but fails because of:
  * Too many requests (429) responses:
```
FILE: ./CHANGELOG.md
[✖] bevyengine#1762 → Status: 429
```
   * crates.io links respond 404
```
FILE: ./README.md
[✖] https://crates.io/crates/bevy → Status: 404
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants