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

Move linkinator to GitHub Actions. #35573

Merged
merged 2 commits into from
Jan 29, 2022
Merged

Move linkinator to GitHub Actions. #35573

merged 2 commits into from
Jan 29, 2022

Conversation

XhmikosR
Copy link
Member

No description provided.

package.json Outdated
@@ -57,7 +57,7 @@
"docs": "npm-run-all docs-build docs-lint",
"docs-build": "hugo --cleanDestinationDir",
"docs-compile": "npm run docs-build",
"docs-linkinator": "linkinator _site --recurse --skip \"^(?!http://localhost)\" --verbosity error",
"docs-linkinator": "npx linkinator _site --recurse --skip \"^(?!http://localhost)\" --verbosity error",
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking perhaps we should remove the npm script completely...

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so, we also have spellcheck and validator here so it makes sense IMHO. Or we may drop all of them ?

Copy link
Member Author

Choose a reason for hiding this comment

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

My plan is to move those to Actions too, but they are not exactly the same in my eyes. cpell can run locally out of the box, linkinator requires a docs build first.

I'm in the middle TBH. I just want to reduce our devDependencies since we do have CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, do note that using npx is not considered a good practice for good reasons.

Copy link
Member

Choose a reason for hiding this comment

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

Then I guess vnu could be dropped too in our devDeps, if it runs in an action already? It also requires a build step I think, and is probably one of our heaviest dependency.

Whatever, drop the linkinator script. This is indeed covered by CI. 👌

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, there's no ready Action to run vnu AFAICT. If there was, I would consider moving that to Actions too.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I thought a bit about this and perhaps the best solution would be to remove the linkinator script from the docs-lint script. It will still be there for now, just not used.

Copy link
Member

Choose a reason for hiding this comment

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

Will we really use it, knowing it's covered by CI? I'm not against keeping it but it looks like dead code if it's not used, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking to keep it until we are sure we are OK with the current approach, but maybe it's fine to just drop it...

@XhmikosR XhmikosR marked this pull request as ready for review December 28, 2021 14:15
@XhmikosR XhmikosR merged commit 640542e into main Jan 29, 2022
@XhmikosR XhmikosR deleted the main-xmr-linkinator-action branch January 29, 2022 12:42
XhmikosR added a commit that referenced this pull request Feb 2, 2022
* Move linkinator to GitHub Actions.

* Remove `docs-linkinator` npm script since it's no longer used
XhmikosR added a commit that referenced this pull request Feb 7, 2022
* Move linkinator to GitHub Actions.

* Remove `docs-linkinator` npm script since it's no longer used
XhmikosR added a commit that referenced this pull request Apr 8, 2022
* Move linkinator to GitHub Actions.

* Remove `docs-linkinator` npm script since it's no longer used
XhmikosR added a commit that referenced this pull request Apr 19, 2022
* Move linkinator to GitHub Actions.

* Remove `docs-linkinator` npm script since it's no longer used
XhmikosR added a commit that referenced this pull request Apr 19, 2022
* Move linkinator to GitHub Actions.

* Remove `docs-linkinator` npm script since it's no longer used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants