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

Add check-external-links command #4235

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

owenatgov
Copy link
Contributor

@owenatgov owenatgov commented Oct 23, 2024

Change

Adds a check-external-links npm script as an offshoot of check-links, creates a manual workflow for it and attempts to fix some initially reported broken links.

This is a reimplementation of #3174 as a separate command. It's inadvisable to include this as a default part of our testing suite because:

  • it takes a long long time to run, roughly 7 minutes
  • it reports on things that aren't actually failues such as links that aren't redirecting or broken but maybe just return unclean responses or some assets that are intentionally not proper links
  • it could raise test failures for PRs that have nothing to do with that PR's change

However a separate comamnd we can run mean we have this tool that we can interpret whenever we want.

An early draft of this PR included the script in our default test suite in order to capture our current broken links. See the original test output of check-external-links here.

The check additionally flagged broken links in our component docs which I've actioned in alphagov/govuk-frontend#5428

Controversial content updates

A few of the content changes I've chosen to make probably need further discussion. I'll note the tricky ones or lingering problems here:

  • In the accessibility statement, we originally linked to the tec docs gem and its accessibility statement, however the website for the library has been taken down. I've instead linked to the github repo and the file in the repo for the statement since its not listed anywhere online.
  • In the code of conduct pages, we link to what was a community policy for Practical Service Design's slack space. However, it looks like Practical are shutting down and have started removing pages, including this one. I've tried to remedy this by removing the link to them entirely.
  • In the footer component, we originally link to a service manual page on publishing info about your services accessibility to help users with adding an accessibility statement. However that page now redirects to info on wcag 2.2. I've tried to remedy this by instead linking to the sample accessibility statement.
  • On the create accounts and national insurance numbers patterns, we had a duplicate section on not using NI numbers as credentials, linking to a section in a service manual page as evidence. That section has now gone from that page. I couldn't find alternative content to fill the gap so I've removed both those sections.
  • On the password input component in the section on not using the spellcheck attribute we mentioned 'spell-jacking'. The page we link to for this is a 404. In fact, it looks like Otto js has completely removed their news section. I tried looking for an alternative but wasn't confident in my choices. I've aleviated this by removing the link entirely.

@owenatgov owenatgov force-pushed the check-external-links-job branch from a66ac71 to b3661f0 Compare October 23, 2024 08:35
Copy link

netlify bot commented Oct 23, 2024

You can preview this change here:

Name Link
🔨 Latest commit a66ac71
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/6718b51820300f0008242386
😎 Deploy Preview https://deploy-preview-4235--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 23, 2024

You can preview this change here:

Name Link
🔨 Latest commit d346675
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/673da6f567da480009f77bf9
😎 Deploy Preview https://deploy-preview-4235--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@owenatgov owenatgov force-pushed the check-external-links-job branch 2 times, most recently from 204fc6a to 6cc2d93 Compare October 24, 2024 08:32
@owenatgov owenatgov marked this pull request as ready for review October 24, 2024 09:01
@owenatgov owenatgov requested a review from a team as a code owner October 24, 2024 09:01
@owenatgov owenatgov force-pushed the check-external-links-job branch from 6cc2d93 to ffb4d09 Compare October 25, 2024 10:03
@owenatgov owenatgov force-pushed the check-external-links-job branch from 06d8e38 to b72e82f Compare October 30, 2024 17:14
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

The command works fine locally, but I think we need a little extra work on the GitHub workflow to avoid issues if the cache has been deleted. Happy to pair if it's useful 😊

Comment on lines 27 to 28
- name: Restore build
uses: actions/cache/[email protected]
with:
key: build-cache-${{ runner.os }}-${{ github.sha }}
path: build
Copy link
Member

Choose a reason for hiding this comment

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

issue This makes the workflow dependent on the build existing in GitHub actions' cache. Caches expire after 7 days without being used.

I think we need to look at sharing the build job between the test and check-external-links workflow. This is something we do in govuk-frontend if we need 'inspiration' 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romaricpascal I've had a go at this in 88178a8. I'm not feeling ultra confident about it, especially since js tests are failing and specifically calling out bad cahcing, which I suspect is the problem. Any advice?

.github/workflows/check-external-links.yaml Outdated Show resolved Hide resolved
.github/workflows/check-external-links.yaml Outdated Show resolved Hide resolved
@owenatgov owenatgov force-pushed the check-external-links-job branch 3 times, most recently from b53996a to 9a3a018 Compare November 5, 2024 15:26
owenatgov and others added 2 commits November 19, 2024 15:57
@owenatgov owenatgov force-pushed the check-external-links-job branch 2 times, most recently from 51a44a4 to 88178a8 Compare November 19, 2024 17:13
@owenatgov owenatgov force-pushed the check-external-links-job branch from 88178a8 to d346675 Compare November 20, 2024 09:08
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.

4 participants