Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

Enable all pre-commit hooks in ci #832

Open
6 tasks
4ydan opened this issue Aug 16, 2023 · 13 comments
Open
6 tasks

Enable all pre-commit hooks in ci #832

4ydan opened this issue Aug 16, 2023 · 13 comments
Assignees
Labels
CI enhancement New feature or request

Comments

@4ydan
Copy link
Contributor

4ydan commented Aug 16, 2023

Task

Instead of invoking each hook individually we could just run pre-commit run -a.
We need to:

  • fix sqlfluff errors/warnings
  • fix hanging of "Fix Lint groovy findings" (maybe use https://github.com/nice-move/prettier-plugin-groovy instead?)
  • install packages for the hook fix-npm-groovy-lint
  • remove cargo-fmt stage to not perform this check twice
  • remove npm linting step inside frontend test stage to not perform this check twice
  • install packages for eslint prettier and fmt hook

This would make the pipeline a bit more transparent, as the stage frontend test does also npm format:check, npm linting and npm test.
And auto include new checks to the pipeline without needing to manually edit the Jenkinsfile.

@4ydan 4ydan added enhancement New feature or request CI low priority labels Aug 16, 2023
@markus2330
Copy link
Contributor

Thank you for reporting this improvement!

we could just run pre-commit run -a.

I fully agree that this is the goal here.

This would make the pipeline a bit more transparent

👍

@markus2330
Copy link
Contributor

@4ydan I think lots of progress happened here? Can you check in the top post what is already done?

@4ydan
Copy link
Contributor Author

4ydan commented Aug 26, 2023

Updated, sadly the other stuff is not that trivial.
We would need to use a dockerfile that has all the backend and frontend stuff needed for the tasks.

@markus2330
Copy link
Contributor

As long as there isn't something that is not checked in the CI pipeline at all (but is checked in pre-commit), I don't worry.

@4ydan
Copy link
Contributor Author

4ydan commented Aug 28, 2023

What do we think about the idea to have a pre-commit file in each subdirectory?
One in backend
One in frontend
One in e2e
One in ci (groovy lint)

This would make the pre-commit file also more readable and enable us to easier integrate it into CI, not needing to build a docker image with the full stack.

@markus2330
Copy link
Contributor

This sounds like a great idea. So the CI would execute the individual checks in each directory (with different Dockerfiles) but for devs it is basically unchanged?

@4ydan
Copy link
Contributor Author

4ydan commented Aug 28, 2023

I hope so, I have to test if it works like I would imagine it to work.
Maybe the first 11 lines are duplicates

# https://pre-commit.com/
repos:
  # General hooks
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.4.0
    hooks:
      - id: check-merge-conflict
      - id: end-of-file-fixer
      - id: mixed-line-ending
      - id: trailing-whitespace
        args: ["--markdown-linebreak-ext=md"]

But else it should hopefully still execute all necessary checks.

Only change for devs is to write new checks in the according .pre-commit-config.yaml
And it would be harder to setup all pre-commit hooks I guess, not sure how good that is.

@4ydan
Copy link
Contributor Author

4ydan commented Aug 28, 2023

Another solution seems to be to use the makefile to create different pre-commit targets.
We could create backend, frontend, etc..

image

@markus2330
Copy link
Contributor

Keeping Makefiles and pre-commit separate sounds better. pre-commit already provides a CLI interface, so I don't see a big benefit in having the targets available in Makefiles, additionally. Or do I miss something important here?

@4ydan
Copy link
Contributor Author

4ydan commented Aug 28, 2023

Yea, I dont like the Makefile idea too much too.
If we are fine increasing the setup complexity, it would be the best splitting the pre-commit file at least in two (one in the backend and one in frontend)
Full stack devs would need to install both, but other than that, it would resolve some of the complexity and enable people to start quicker developing with a subproject when everything that is needed in that subfolder is where it is.

@4ydan
Copy link
Contributor Author

4ydan commented Aug 28, 2023

I just realized this would only work if the subfolders are git submodules or git repos. So we cant do this, as multiple installations of pre-commits are not possible, they overwrite each other.

@markus2330
Copy link
Contributor

Let us rest this issue again a bit. Let us pick up again when something changes in pre-commit.

@markus2330
Copy link
Contributor

markus2330 commented Apr 19, 2024

Currently sql format fails again and Fix Lint groovy findings hangs. So we should get this fixed in CI, to avoid formatting errors creeping in again.

I added (unticked) to top post:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

3 participants