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

feat: replace super-linter with custom linter list #97

Merged
merged 35 commits into from
May 24, 2023

Conversation

ChiefHolland
Copy link
Contributor

@ChiefHolland ChiefHolland commented May 12, 2023

Contributor Comments

This PR removes the super-linter and replaces it with a list of linters in etc/linters.json. Configuration files for each linter were created in etc/ and match the configs contained in the super-linter. Logic was included to allow custom arguments to be passed in for each linter in an environment variable, as well as a regex-based file exclusion list.

Pull Request Checklist

Thank you for submitting a contribution to the goat!

In order to streamline the review of your contribution we ask that you review and comply with the below requirements:

  • Rebase your branch against the latest commit of the target branch
  • If you are adding a dependency, please explain how it was chosen
  • If manual testing is needed in order to validate the changes, provide a testing plan and the expected results
  • If there is an issue associated with your Pull Request, link the issue to the PR.

@ChiefHolland ChiefHolland changed the title Replace super linter feat: replace super-linter wtih custom linter list May 12, 2023
@ChiefHolland ChiefHolland changed the title feat: replace super-linter wtih custom linter list feat: replace super-linter with custom linter list May 12, 2023
@ChiefHolland ChiefHolland marked this pull request as ready for review May 17, 2023 21:21
.github/etc/dictionary.txt Outdated Show resolved Hide resolved
.github/workflows/commit.yml Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
etc/linters.json Outdated Show resolved Hide resolved
entrypoint.sh Show resolved Hide resolved
entrypoint.sh Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
ChiefHolland and others added 2 commits May 18, 2023 14:52
Co-authored-by: JonZeolla <[email protected]>
Co-authored-by: JonZeolla <[email protected]>
entrypoint.sh Show resolved Hide resolved
entrypoint.sh Show resolved Hide resolved
entrypoint.sh Outdated Show resolved Hide resolved
@JonZeolla JonZeolla mentioned this pull request May 21, 2023
4 tasks
Copy link
Member

@JonZeolla JonZeolla left a comment

Choose a reason for hiding this comment

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

Quick review; will need one more full run through of the actual business logic but wanted to get you this feedback sooner

Dockerfile Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
entrypoint.sh Outdated Show resolved Hide resolved
etc/linters.json Outdated Show resolved Hide resolved
entrypoint.sh Show resolved Hide resolved
entrypoint.sh Outdated Show resolved Hide resolved
entrypoint.sh Outdated Show resolved Hide resolved
entrypoint.sh Show resolved Hide resolved
@ChiefHolland ChiefHolland enabled auto-merge (squash) May 23, 2023 23:53
Copy link
Member

@JonZeolla JonZeolla left a comment

Choose a reason for hiding this comment

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

Sorry, I need to un-approve just because it introduces breaking changes when I tested it against some projects.

It scans easy_infra in 7 seconds 👍👍, but fails the build due to the shfmt tabs and cfn-lint issues; we need a way to turn these off or loosen the shfmt rules for now.

We may be able to add an .editorconfig file to the goat/etc/ folder and configure it in line with https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd but I can't get it to work.

I suggest scanning some other projects manually and see what happens

Also, when we skip a linter we need to add a WARNING: to the end that it was skipped (I did this in 5f92079)

@JonZeolla
Copy link
Member

JonZeolla commented May 24, 2023

I don't know how to get it to find the .editorconfig because they don't seem to have a way to pass an arbitrary location to load it from. It loads from current directory up to the root - see https://editorconfig.org/#file-location

That will also cause an issue for the .github/linters/ config override approach.

Perhaps the easiest approach is just remove shfmt for now and put it on the backlog?

BTW it looks like there was a config for this in super-linter here.

@JonZeolla
Copy link
Member

We don't need to change cfn-lint; I made the recommended changes

Copy link
Member

@JonZeolla JonZeolla left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@ChiefHolland ChiefHolland merged commit 053a94d into main May 24, 2023
@ChiefHolland ChiefHolland deleted the replace-super-linter branch May 24, 2023 23:46
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