[lint-staged] Add ShellCheck to `.lintstagedrc.js`
ClosedPublic
Actions

Authored by • abosh on Aug 8 2022, 9:41 AM.

Details

Reviewers

atul
• jon
inka

Commits

rCOMMdbfcd8204985: [lint-staged] Add ShellCheck to `.lintstagedrc.js`

Summary

Related Linear issue here. Adds a task for handling *.sh staged files and runs the same ShellCheck command we use in Buildkite/GitHub Actions. Because this we're running ShellCheck in the pre-commit stage, issues with shell scripts will be surfaced to developers earlier, which is ideal.

Depends on D4771 because the user needs ShellCheck installed locally.

Also depends on D4770 (related issue: ENG-1568) which adds ShellCheck to the Nix shell for developers using the Nix shell.

Test Plan

Tested locally on macOS and works as expected:

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• abosh created this revision.Aug 8 2022, 9:41 AM

Herald added subscribers: • adrian, tomek, ashoat. · View Herald TranscriptAug 8 2022, 9:41 AM

• abosh edited the summary of this revision. (Show Details)Aug 8 2022, 9:43 AM

• abosh edited the test plan for this revision. (Show Details)

• abosh mentioned this in D4771: [docs] Add ShellCheck installation instructions.

https://phab.comm.dev/D4770 should probably be merged before this, not sure how to handle mult-owner stacks though

Harbormaster completed remote builds in B11214: Diff 15426.Aug 8 2022, 9:55 AM

• abosh requested review of this revision.Aug 8 2022, 9:55 AM

In D4772#137507, @jon wrote:

https://phab.comm.dev/D4770 should probably be merged before this, not sure how to handle mult-owner stacks though

Looks like I can add a depends on rule to this diff, which I just did! So it's part of the stack now.

• abosh edited the summary of this revision. (Show Details)Aug 8 2022, 9:57 AM

• abosh edited the summary of this revision. (Show Details)

Before I accept, could you send a message in the Comm Dev Team chat letting everyone know that they'll need to install shellcheck (and provide instructions on how they can go about doing that?)

This revision now requires changes to proceed.Aug 8 2022, 10:50 AM

In D4772#137561, @atul wrote:

Before I accept, could you send a message in the Comm Dev Team chat letting everyone know that they'll need to install shellcheck (and provide instructions on how they can go about doing that?)

Yup, was planning on doing this, however want to wait for D4771 to get landed so I can additionally just link to the dev environment doc for "official" documentation. But will definitely do this once D4771 is landed.

In D4772#137580, @abosh wrote:

In D4772#137561, @atul wrote:

Before I accept, could you send a message in the Comm Dev Team chat letting everyone know that they'll need to install shellcheck (and provide instructions on how they can go about doing that?)

Yup, was planning on doing this, however want to wait for D4771 to get landed so I can additionally just link to the dev environment doc for "official" documentation. But will definitely do this once D4771 is landed.

I think you can just tell people to run brew install shellcheck without waiting for anything to land?

atul requested changes to this revision.Aug 8 2022, 11:08 AM

This revision now requires changes to proceed.Aug 8 2022, 11:08 AM

Ok, will do.

Sent message.

• abosh retitled this revision from [lint-staged] Add ShellCheck to `.lint-staged.js` to [lint-staged] Add ShellCheck to `.lintstagedrc.js`.Aug 8 2022, 12:55 PM

Looks good, but lets hold off on landing this until nix support gets landed

This revision is now accepted and ready to land.Aug 8 2022, 1:05 PM

In D4772#137753, @atul wrote:

Looks good, but lets hold off on landing this until nix support gets landed

I agree, added D4770 to this stack for that reason.

In D4772#137759, @abosh wrote:

In D4772#137753, @atul wrote:

Looks good, but lets hold off on landing this until nix support gets landed

I agree, added D4770 to this stack for that reason.