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

ci(lint): use super-linter #1371

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ttshivers
Copy link
Member

@ttshivers ttshivers commented Oct 19, 2020

Replace all other linters with super-linter (docs here): https://github.com/github/super-linter
It has all the linters that the previous actions used.

Docs for disabling rules: https://github.com/github/super-linter/blob/master/docs/disabling-linters.md

Refs: #1368 (comment)

@ttshivers ttshivers force-pushed the super_linter branch 3 times, most recently from 0c251c4 to 430e0b3 Compare October 19, 2020 06:22
@SimenB
Copy link
Member

SimenB commented Oct 19, 2020

This is great! Huge +1. Would you be up for fixing the violations as well? 🙂

@ttshivers
Copy link
Member Author

Sure. I can start off with the md files. Unsure about whether the Dockerfile lint errors should be fixed.

@SimenB
Copy link
Member

SimenB commented Oct 19, 2020

Don't see a reason why they shouldn't?

/cc @nodejs/docker

@PeterDaveHello
Copy link
Member

I vote -0 here, there are many details in super-linter we may need to control by ourselves, like the versions of the linters, the excluded rules of the linters, etc.

@ttshivers ttshivers closed this Oct 19, 2020
@ttshivers ttshivers reopened this Oct 19, 2020
@ttshivers
Copy link
Member Author

Some of the current errors:

hadolint (docker):

/github/workspace/12/alpine3.12/Dockerfile:5 DL3003 Use WORKDIR to switch to a directory
/github/workspace/12/alpine3.12/Dockerfile:5 DL3018 Pin versions in apk add. Instead of `apk add <package>` use `apk add <package>=<version>`
/github/workspace/12/alpine3.12/Dockerfile:5 DL4006 Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check
/github/workspace/12/alpine3.12/Dockerfile:76 SC2043 This loop will only ever run once. Bad quoting or missing glob/expansion?

Unsure if some of these should be fixed or not like whether the alpine apk packages be pinned?

shfmt: It looks like it prefers no space after the redirection. Errors are mostly all like this.

-hash git 2> /dev/null || { echo >&2 "git not found, exiting."; }
+hash git 2>/dev/null || { echo >&2 "git not found, exiting."; }

js (standard). All semicolon or comma:

/github/workspace/genMatrix.js:3:25: Extra semicolon.
/github/workspace/genMatrix.js:7:37: Unexpected trailing comma.

markdownlint. Misc errors:

/github/workspace/README.md:61:1 MD014/commands-show-output Dollar signs used before commands without showing output [Context: "$ docker build -t my-nodejs-ap..."]
/github/workspace/README.md:245 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "#### Docker Working Group Memb..."]]�[0m
/github/workspace/docs/BestPractices.md:41 MD028/no-blanks-blockquote Blank line inside blockquote
/github/workspace/docs/BestPractices.md:86 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
/github/workspace/docs/BestPractices.md:173:149 MD034/no-bare-urls Bare URL used [Context: "https://github.com/docker/dock..."]]�[0m

@nschonni
Copy link
Member

It might help to add some problem matches so the specific lines are highlighted.

@SimenB
Copy link
Member

SimenB commented Oct 21, 2020

Sorta odd the "super" linter doesn't come with problem matchers

@ttshivers ttshivers force-pushed the super_linter branch 4 times, most recently from 6c21201 to 28ed739 Compare October 22, 2020 03:36
@nschonni
Copy link
Member

Looks like it won't do more than 10 annotations for the non-changed files, but that's probably OK

@ttshivers ttshivers force-pushed the super_linter branch 12 times, most recently from a28dd5b to 818c8f4 Compare October 22, 2020 05:09
@nschonni
Copy link
Member

I'm thinking that for the This loop will only ever run once. Bad quoting or missing glob/expansion? for the Yarn keys. Maybe we should just flatten the loop, since there only appears to be a single signing key for them

@ttshivers
Copy link
Member Author

I'm thinking that for the This loop will only ever run once. Bad quoting or missing glob/expansion? for the Yarn keys. Maybe we should just flatten the loop, since there only appears to be a single signing key for them

Yeah that is doable. I could more easily do this when my node port to update.js is finished.

@PeterDaveHello
Copy link
Member

We use shfmt as shfmt -sr -i 2 -l -w -ci ., so there are some different places here, I prefer the original parameters we're using right now, instead of fix/reformat those scripts.

Also wondering, if super-linter updated a version of the linters which need our changes, but we don't have the effort to resolve it immediately, it could be a problem, as super-linter doesn't seem to be able to specify the version of the linters.

@ttshivers
Copy link
Member Author

ttshivers commented Oct 23, 2020

We use shfmt as shfmt -sr -i 2 -l -w -ci ., so there are some different places here, I prefer the original parameters we're using right now, instead of fix/reformat those scripts.

Also wondering, if super-linter updated a version of the linters which need our changes, but we don't have the effort to resolve it immediately, it could be a problem, as super-linter doesn't seem to be able to specify the version of the linters.

Sure, I think I could make shfmt use those parameters. I think it follows the editorconfig: https://github.com/github/super-linter/blob/master/docs/disabling-linters.md#shfmt-config-file

As for the versioning, the current actions (except for shfmt) aren't pinned atm. The new action pins super-linter so I wouldn't expect any non-major version increase to introduce any breaking changes.

@ttshivers ttshivers force-pushed the super_linter branch 2 times, most recently from 7b56b85 to 589edd1 Compare October 23, 2020 04:53
@PeterDaveHello
Copy link
Member

Sure, I think I could make shfmt use those parameters. I think it follows the editorconfig: https://github.com/github/super-linter/blob/master/docs/disabling-linters.md#shfmt-config-file

If we use the same parameters, I think the shell scripts should be the same without any change? Looks like the indentation was still revised 🤔

As for the versioning, the current actions (except for shfmt) aren't pinned atm. The new action pins super-linter so I wouldn't expect any non-major version increase to introduce any breaking changes.

Cool 👍

@ttshivers ttshivers force-pushed the super_linter branch 2 times, most recently from 00f7976 to 2e23cbd Compare November 14, 2020 19:32
Replace all other linters with super-linter
Base automatically changed from master to main March 15, 2021 16:23
@SimenB
Copy link
Member

SimenB commented Mar 10, 2022

Can we revive this? And also set JAVASCRIPT_DEFAULT_STYLE to prettier instead of standard. And the slim image

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