Add broken link checker - #958
Conversation
✅ Deploy Preview for cal-itp-mobility-marketplace ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jgravois
left a comment
jgravois
left a comment
There was a problem hiding this comment.
looks promising!
i'll hold off on reviewing this one until i see a completed workflow run.
If an open issue with label `linkchecker` is found, it will post a new comment to it. Otherwise, it will create a fresh issue.
ab14ac1 to
49df560
Compare
|
Thought I had left this in draft, but apparently not. It's now ready for review! For the PR check, I elected to just have it run like any other PR check and report success or failure (i.e., it doesn't post a comment as I previously suggested doing). You can see it doing this on this very PR. For the weekly run, the approach I took was to have it open an issue with a copy of the report if there are any failures. If it finds that there is already an issue with the You can see an example of how it looks in the test repo: compilerla/actions-testing#166 Feedback welcome! |
There was a problem hiding this comment.
i have some feedback that is orthogonal to something i said inline.
personally, i think the cron workflow is substantially higher value here than the action that runs on pull_request. if we aren't catching broken links during manual QA, we have bigger problems.
| - "eleventy.config.js" | ||
| - "linkcheckerrc" | ||
| - "src/**" |
There was a problem hiding this comment.
with this many path filters, i'd be inclined to just yank paths altogether.
There was a problem hiding this comment.
I really prefer not to run unnecessary checks when we can avoid them without too much effort. I don't think we actually have to pay for Actions minutes since our repos are open source, but it's still wasteful and adds noise on PRs where they don't apply.
There was a problem hiding this comment.
that's why my orthogonal suggestion was to get rid of the workflow altogether! 😆
There was a problem hiding this comment.
That's a solid point! I'm not opposed to not doing it on PRs of everyone agrees a weekly check is sufficient.
| jobs: | ||
| linkchecker: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 # prevent background server step from running forever if something else goes wrong |
There was a problem hiding this comment.
this reminds me that it would be cool to have our new terraform gh actions self-destruct and ensure their associated locks are removed....
| - name: Check for an open linkchecker issue | ||
| id: open-issue-check | ||
| if: ${{ failure() }} | ||
| run: echo "OPEN_ISSUE_ID=`gh issue list --label linkchecker --json number --jq '.[0].number'`" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
i dig that you're just using gh for all this.
There was a problem hiding this comment.
It's a pretty powerful addition to the built-in GHA toolset!
| --body-file comment.md \ | ||
| --repo $GITHUB_REPOSITORY |
There was a problem hiding this comment.
YMMV, but in peter-evans/create-or-update-comment, the default edit-mode mode appends files to anything existing it finds...
There was a problem hiding this comment.
Thanks for suggesting this. I have a few reasons for preferring the current approach, though:
- We have expressed a desire to reduce our allowance on third-party actions.
- It's not even clear to me from the docs/issues in that repo if that action can update the original issue description, or just comments. Is the description considered a comment that can be acted on in the same way? Would require some deeper research and/or testing.
- Most importantly (to me), editing the issue description/comment will not create a new notification for those watching the issue, which will make it harder to notice new results that occur after someone has already started working on the prior results (or make it easy to forget how long reported issues have going unaddressed).
There was a problem hiding this comment.
sorry. i didn't intend to suggest that you switch to the github action i linked to.
i was only saying that the underlying gh behavior might be the same. 😇
There was a problem hiding this comment.
Ah!
I wouldn't expect that to be the case since I'm just telling it to make a new comment and not referencing an exiting comment, but I'll run the workflow in the test repo manually again and confirm what happens.
There was a problem hiding this comment.
Confirmed: compilerla/actions-testing#166 (comment)
| ignore= | ||
| /cdn-cgi/l/email-protection |
There was a problem hiding this comment.
what are these links pray tell?
There was a problem hiding this comment.
Turns out that we have Cloudflare set up to mask email addresses, so the href that linkchecker sees for a mailto link looks like this:
<a href="/cdn-cgi/l/email-protection#79111c151516391a1815100d0957160b1e">
<span class="__cf_email__" data-cfemail="670f020b0b082704060b0e131749081500">[email protected]</span>
</a>And those result in 404s if it just tries to follow them directly.
thekaveman
left a comment
thekaveman
left a comment
There was a problem hiding this comment.
Just to make it explicit, we are awaiting removal of the PR workflow and aligned on the weekly automated workflow only 👍
3 participants
Part of #962