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

feat: check links via linkinator #255

Merged
merged 6 commits into from
Dec 5, 2023
Merged

feat: check links via linkinator #255

merged 6 commits into from
Dec 5, 2023

Conversation

mshanemc
Copy link
Contributor

check markdown files for broken links

  1. top-level of the project that are not CHANGELOG.md
  2. and everything in /messages

@W-14543815@

@@ -47,14 +47,20 @@ const PACKAGE_DEFAULTS = {
files: ['src/**/*.ts', 'test/**/*.ts', 'messages/**', '**/.eslint*', '**/tsconfig.json'],
output: [],
},
'link-check': {
command:
'linkinator "./!(CHANGELOG).md" "messages/**/*.md" --markdown --retry --directory-listing --verbosity error',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

different glob implementations. the command uses ! to get everything that isn't the changelog.

the files in wireit uses the ! to exclude thing included by the previous globs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was playing around with this a bit. We are missing some files with this current pattern. What do you think about this:

"command": "linkinator \"**/*.md\" --skip 'CHANGELOG.md|node_modules|^test|confluence.internal.salesforce.com' --markdown --retry --directory-listing --verbosity error",

This finds an additional 20 links in forcedotcom/source-deploy-retrieve (from 50 to 70). We can add additional "skips" as we find them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commit that!

@iowillhoit
Copy link
Contributor

I wonder if we should come up with a way to skip this if needed. Maybe with an environment variable? I am imagining this exiting on a false positives (website down) and preventing a release. You could force merge a PR but you would not have any NUTs run if the unit tests failed because of linkinator. Example GHA

@mshanemc
Copy link
Contributor Author

mshanemc commented Dec 5, 2023

I wonder if we should come up with a way to skip this if needed. Maybe with an environment variable? I am imagining this exiting on a false positives (website down) and preventing a release. You could force merge a PR but you would not have any NUTs run if the unit tests failed because of linkinator. Example GHA

Yeah, so the GHA could read from GHA variables to skip itself? Good idea, commit that, too.

@iowillhoit
Copy link
Contributor

Ok @mshanemc this is ready for you to take a look at. I made a couple more changes to this.

  • Changed ^test to test/. Despite what their docs say, I could not get this regex to work.
    • The downside of this is that it will also skip nested test directories. For example messages/test/foo.md
  • Added %s like we use in plugin-info
  • Added link-check to the scripts so you can run it on its own
  • Added a cross-os way of skipping the link-check. See screenshot
Screenshot 2023-12-05 at 3 20 28 PM

@iowillhoit
Copy link
Contributor

Hold up a bit on this, the env var is not working in CI. Trying a couple things.

@mshanemc mshanemc merged commit 5c5a1b4 into main Dec 5, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants