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

Consolidate linters with github/super-linter. #103

Closed
wants to merge 3 commits into from
Closed

Conversation

sengi
Copy link
Contributor

@sengi sengi commented Jun 28, 2024

This results in much less config and way better capability. It's slightly slower because of the image pull (not sure why it's not cached better) but it still takes under a minute so I think it's worth it since it's way easier to enable other linters in future and eliminates a lot of maintenance.

Alternatives considered:

  • Keep maintaining our own glue for each linter: runs slightly faster cos no big OCI image to download, but maintenance is toily and the overall solution is brittle and inflexible (huge pain to add new linters).
  • Reviewdog: similar, looks fine, but somewhat more third-party and slightly less widely used.

Neither super-linter nor Reviewdog seems to integrate with GitHub problem matchers yet, so holding off on this for now since we currently have problems matchers working (i.e. findings shown inline with code in the GitHub review UI) and they're pretty valuable.

@sengi sengi requested review from dj-maisy and theseanything June 28, 2024 16:45
theseanything
theseanything previously approved these changes Jul 8, 2024
@sengi sengi marked this pull request as draft July 8, 2024 12:31
@sengi sengi force-pushed the sengi/actionlint branch 5 times, most recently from f228b06 to 8a80276 Compare July 8, 2024 13:00
@sengi sengi marked this pull request as ready for review July 8, 2024 13:16
sengi added 3 commits July 8, 2024 14:16
This gets rid of a lot of ugly glue that we'd otherwise have to continue
maintaining ourselves.

Enable the linters we we running before, plus actionlint.
@sengi sengi force-pushed the sengi/actionlint branch from 8a80276 to 61ad040 Compare July 8, 2024 13:18
@sengi
Copy link
Contributor Author

sengi commented Jul 8, 2024

Rather than faff about getting problem-matchers to work, decided to look around and found we can replace all our home-grown linter actions glue with github/super-linter.

PTAL

@sengi sengi changed the title Run actionlint as a pre-merge check. Consolidate linters with github/super-linter + enable actionlint. Jul 8, 2024
@sengi sengi requested a review from samsimpson1 July 8, 2024 13:27
@sengi sengi force-pushed the sengi/actionlint branch from 8b31e15 to 61ad040 Compare July 8, 2024 13:40
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I verified that it really does require this, even though we're not currently using any of the linters which use the commit history 🤷

sengi added a commit to alphagov/govuk-infrastructure that referenced this pull request Jul 8, 2024
sengi added a commit to alphagov/govuk-infrastructure that referenced this pull request Jul 8, 2024
@sengi sengi marked this pull request as draft July 8, 2024 14:56
@sengi sengi changed the title Consolidate linters with github/super-linter + enable actionlint. Consolidate linters with github/super-linter. Jul 8, 2024
@sengi sengi dismissed theseanything’s stale review July 8, 2024 16:02

changed scope and probably won't merge this soon, thanks for the review tho

@sengi sengi removed request for samsimpson1 and dj-maisy July 8, 2024 16:02
@sengi sengi closed this Jul 8, 2024
@sengi sengi deleted the sengi/actionlint branch July 8, 2024 16:17
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.

2 participants