Skip to content

chore: fix linting issues to allow upgrade to super-linterv6 #38

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 21, 2024
Merged

chore: fix linting issues to allow upgrade to super-linterv6 #38

merged 1 commit into from
Mar 21, 2024

Conversation

jmeridth
Copy link
Member

@jmeridth jmeridth commented Mar 20, 2024 •
edited
Loading

Closes #19

Pull Request

Proposed Changes

  • add skips for checkov
    • can't add user due to needing root access context
    • add health check endpoint - not there yet
  • move flake8 arguments from Makefile into .github/linters/.flake8 config file
    • use .flake8 config in Makefile
  • move .pylintrc from root to .github/linters/.pylintrc so superlinter uses it
    • rename file to .python-lint (default that superlinter looks for)
  • add missing --- at top of stale.yml github action (makes warning happy)
  • update super-linter to v6
  • update README with permissions best practices in examples

Fixes based on the following errors:

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run make lint and fix any issues that you have introduced
  • run make test and ensure you have test coverage for the lines you are introducing

Reviewer

  • Label as either bug, documentation, enhancement, infrastructure, or breaking

@jmeridth jmeridth requested a review from zkoppert as a code owner March 20, 2024 07:44
zkoppert
zkoppert approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@zkoppert zkoppert left a comment

Choose a reason for hiding this comment

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

✨ 🚢

@zkoppert
Copy link
Member

@jmeridth Are these linting errors with isort something that V6 of the super linter looks at differently?

@jmeridth jmeridth force-pushed the jm-fix-lint-issues-to-allow-upgrade-to-superlinter6 branch from 0cf6771 to 472e461 Compare March 20, 2024 18:41
@jmeridth
Copy link
Member Author

jmeridth commented Mar 20, 2024 •
edited
Loading

@zkoppert linting passed. This is with super-linter v5 though, like you pointed out. Maybe I make the upgrade as part of this PR and see what happens. 🤔

@jmeridth jmeridth force-pushed the jm-fix-lint-issues-to-allow-upgrade-to-superlinter6 branch from 472e461 to c9e5c69 Compare March 20, 2024 18:52
@jmeridth
Copy link
Member Author

jmeridth commented Mar 20, 2024 •
edited
Loading

updated super-linter to v6

@jmeridth
Copy link
Member Author

oK. Going to fix these new linting issues.

@jmeridth jmeridth force-pushed the jm-fix-lint-issues-to-allow-upgrade-to-superlinter6 branch 2 times, most recently from f29e44d to 9d3eced Compare March 20, 2024 19:28
@jmeridth
Copy link
Member Author

jmeridth commented Mar 20, 2024 •
edited
Loading

Even after adding the install dependencies to the linting workflow, the super-linter is reporting unable to find 3rd party package imports. 🤔

Looks like we have to explicitly tell super-linter where to find the site-packages. Gonna try

@jmeridth jmeridth force-pushed the jm-fix-lint-issues-to-allow-upgrade-to-superlinter6 branch 4 times, most recently from 907ccb9 to 46c52a1 Compare March 20, 2024 21:01
@jmeridth
chore: fix linting issues to allow upgrade to supelinter6
4dfccdf 
- [x] add skips for checkov
  - can't add user due to needing root access [context](github/stale-repos#103)
  - add health check endpoint - not there yet
- [x] move flake8 arguments from Makefile into .github/linters/.flake8 config file
  - use .flake8 config in Makefile
- [x] move .pylintrc from root to .github/linters/.pylintrc so superlinter uses it
  - rename file to .python-lint (default that superlinter looks for)
- [x] add missing --- at top of stale.yml github action (makes warning happy)
- [x] update super-linter to v6
- [x] update README with permissions best practices in examples

there might be a few other linting errors, but I'm not able to recreate locally currently

Signed-off-by: jmeridth <jmeridth@gmail.com>
@jmeridth jmeridth force-pushed the jm-fix-lint-issues-to-allow-upgrade-to-superlinter6 branch from 46c52a1 to 4dfccdf Compare March 20, 2024 21:04
@jmeridth
Copy link
Member Author

jmeridth commented Mar 20, 2024 •
edited
Loading

Ok. That was painful. I realized we had this passing in github.com/github/stale-repos. I borrowed it's .github/linters/.python-lint and added the ones we needed. Having import-error included bugs me but super-linter doesn't handle package imports well because it doesn't reuse the checkout/dependencies install locations (it seems). Going to look into that more later.

@zkoppert this is good to merge IMO. and we can close #19 if it doesn't auto-close.

@jmeridth jmeridth changed the title chore: fix linting issues to allow upgrade to supelinter6 chore: fix linting issues to allow upgrade to super-linterv6 Mar 20, 2024
@zkoppert zkoppert merged commit b7d5fa0 into github:main Mar 21, 2024
@zkoppert
Copy link
Member

Generally I think we will catch import errors in our testing so I'm not too stressed about it but it would be nice if this stuff would work as advertised. 😓

@jmeridth jmeridth deleted the jm-fix-lint-issues-to-allow-upgrade-to-superlinter6 branch March 21, 2024 06:41
@jmeridth
Copy link
Member Author

The super-linter arm64 progress seems to be going well. Eventually we'll just run it in a container locally and match the action. Goals and dreams. Goals and dreams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkoppert zkoppert zkoppert approved these changes

Assignees
No one assigned
Labels
infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jmeridth @zkoppert