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

Set up linters on github to check PRs #124

Closed
1 task done
fyliu opened this issue Jan 5, 2023 · 10 comments · Fixed by #230
Closed
1 task done

Set up linters on github to check PRs #124

fyliu opened this issue Jan 5, 2023 · 10 comments · Fixed by #230
Assignees

Comments

@fyliu
Copy link
Member

fyliu commented Jan 5, 2023

Overview

In the interest of all devs, we should set up a style checker on the github side. There's several cases of code in main with inconsistent formatting that could have been prevented by running linters before committing or pushing, like what pre-commit provides automatically.

Action Items

  • setup pre-commit.ci

Resources/Instructions

  • as much as possible, run the same checks on ci as locally.
@fyliu fyliu added size: 0.5pt Can be done in 2-3 hours role: repo feature: infrastructure For changes on site technical architecture feature: process improvement labels Jan 5, 2023
@ExperimentsInHonesty ExperimentsInHonesty added this to the 5 - Team Workflow milestone Mar 4, 2023
@ExperimentsInHonesty ExperimentsInHonesty added role: missing s: PD team stakeholder: People Depot Team labels Mar 5, 2023
@ExperimentsInHonesty ExperimentsInHonesty removed the feature: infrastructure For changes on site technical architecture label Jun 29, 2023
@fyliu
Copy link
Member Author

fyliu commented Aug 4, 2023

We discussed this and we would like to use super linter

@fyliu fyliu added size: 1pt Can be done in 4-6 hours and removed size: 0.5pt Can be done in 2-3 hours labels Aug 4, 2023
@AzaniaBG AzaniaBG self-assigned this Nov 17, 2023
@AzaniaBG
Copy link
Member

AzaniaBG commented Dec 1, 2023

Update: I have reviewed some documentation and have a few questions for @fyliu on implementation:

  1. Do we only want this action to run on a PR to the main branch?
  2. Do we only need the following checked?
  • MD files
  • Python
  • black
  • flake8

@fyliu
Copy link
Member Author

fyliu commented Dec 1, 2023

  1. Yes only on PRs to main

  2. Preferably everything that pre-commit checks. This is in case someone didn't install pre-commit locally or if they bypassed the checks using --no-verify.

So

  • Dockerfile
  • Python (black, flake8, isort)
  • scan for secrets
  • md (not sure if we scan these but I manually run mdformat on my commits. I haven't been able to get it fully functional to add it as an automation)
  • general file checks like remove trailing spaces, add blank line at the end of file
  • and more checks. The config is here

Actually, it might be better/easier to just use the pre-commit bot to do the checks. It's a better mirror of the checks we do locally before committing and pushing. No surprises waiting at the GitHub PR if the local pre-commit checks all pass.

It's free for public repos. I have it set up in my fork and it reads the config file and runs in the PR. There's still some issue with at least one of the checks, so it's running but not fully working for me.

The problem one is, which checks Dockerfile, and I know it's available in superlinter. Maybe we can use both to cover everything?

@fyliu
Copy link
Member Author

fyliu commented Dec 5, 2023

@AzaniaBG I found a way to run the rest of the checks using the pre-commit service. See them run in this demo PR
2023-12-05 12 51 06 github com 16e34b4d0f55
2023-12-05 12 54 31 results pre-commit ci 09a38015d846

@AzaniaBG
Copy link
Member

AzaniaBG commented Dec 5, 2023

@fyliu, thanks for the update. Does that mean super-linter is not required here?

@fyliu
Copy link
Member Author

fyliu commented Dec 5, 2023

@AzaniaBG Yes. I think we should just use pre-commit both in github and locally since they can both run using the same config file.

The only thing left in the pre-commit config that's runnable from github is pytest. That probably needs a separate issue since people have so many strategies to run it on github it probably takes time to figure out which one we should use.

@AzaniaBG
Copy link
Member

AzaniaBG commented Dec 7, 2023

@fyliu ~ Thanks, we can discuss setting up a new issue (per your last comment) at today's meeting.

@fyliu
Copy link
Member Author

fyliu commented Dec 7, 2023

@AzaniaBG hackforla cancelled all the December meetings to let the volunteers recharge. Participation would probably be spotty anyway.

Anywya, it's basically this. But it looks like we need to make the project run-able without docker for this type of GHA to work. I have an issue to set it up using poetry which is #178.

@ethanstrominger got it to run and he's using another tool called pur. I think it's still better to use poetry at this point. Its functionality includes dependency management and creating the virtual environment for running the project. It also lets us separate dev vs. production dependencies.

@fyliu fyliu assigned fyliu and unassigned AzaniaBG Dec 15, 2023
@fyliu
Copy link
Member Author

fyliu commented Dec 15, 2023

I'm changing the issue to one using pre-commit.ci.

I didn't know about it at them time of creating this issues, and my thoughts were to run the same checks as the local pre-commit using something like superlinter. But it's pretty hard to hit all the same checks.

Recently, I was reading the Git DX book which mentioned pre-commit.ci and how to use it. It uses the same config file as the local pre-commit setup, so most of it just worked when I thought I was going to try it for fun. It was so easy and the checks are exactly the same as the local ones that it became clear that we should be using it. It's also faster than the competition.

@fyliu fyliu mentioned this issue Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅Done
Development

Successfully merging a pull request may close this issue.

3 participants