crocs-muni / sec-certs

Tool for analysis of security certificates and their security targets (Common Criteria, NIST FIPS140-2...).
https://sec-certs.org
MIT License
12 stars 8 forks source link

Replace lint action with pre-commit action #310

Closed adamjanovsky closed 1 year ago

adamjanovsky commented 1 year ago

This is to address problem that I recently discovered in our QA approach.

Problem

Solution

A proposed solution is to leave the linting to pre-commit altogether:

The revisions of pre-commit tools can be periodically (but manually) updated with pre-commit autoupdate

Bonus: It's also faster.

Only downside that I can think of is an integration of the individual tools (black, mypy, ruff) with IDEs. If those IDEs rely on running them from the virtual environment in which sec-certs is installed, they just lost the source of which versions to install (as this will only be mentioned in pre-commit-config).

adamjanovsky commented 1 year ago

This also deletes pylint rules from ruff due to detected unstable behaviour. Pylint is quite new thing in ruff and some rules work only on some invocations on ruff leading to unpredictable behaviour. I don't think we'll suffer that much by that.

adamjanovsky commented 1 year ago

@J08nY thoughts?

codecov[bot] commented 1 year ago

Codecov Report

Base: 73.04% // Head: 73.04% // No change to project coverage :thumbsup:

Coverage data is based on head (f09bfa2) compared to base (3ed9d4d). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #310 +/- ## ======================================= Coverage 73.04% 73.04% ======================================= Files 45 45 Lines 5545 5545 ======================================= Hits 4050 4050 Misses 1495 1495 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=crocs-muni). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=crocs-muni)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

J08nY commented 1 year ago

I don't have a strong opinion. I only find it a bit iffy because:

adamjanovsky commented 1 year ago

I don't have a strong opinion. I only find it a bit iffy because:

* Installing the dev-reqs does not actually install the linters as one might expect.

* Also this sort of binds us with "pre-commit actions == CI linting" which might not be what we want in the future. For example, we might want to use some other/custom pre-commit hooks to do something and doing it in CI would not be good.
  But I also see the good in this, that we actually get the same behavior and versions in CI vs pre-commit.

All are very valid comments that didn't move us any closer to a resolution :D

Just for the record, current situation is that linter versions must be manually synced across three files (pre-commit, lint.yml, pyproject.toml).

Also, the pre-commit guy says that he uses such setup https://stackoverflow.com/questions/65940222/pre-commit-com-same-version-in-pre-commit-config-yaml-and-requirements-txt, which may or may not be relevant.

Same question on pre-commit https://github.com/pre-commit/pre-commit/issues/2535

Workaround without commiting purely to pre-commit is installing all dev-requirements in all lint Github actions, pinning linters in pyproject.toml and then manually syncing two files (pyproject.toml and pre-commit-config.yaml) from time to time, e.g. before each release.

J08nY commented 1 year ago

Workaround without commiting purely to pre-commit is installing all dev-requirements in all lint Github actions, pinning linters in pyproject.toml and then manually syncing two files (pyproject.toml and pre-commit-config.yaml) from time to time, e.g. before each release.

I actually thought this was what was already being done :)

Btw. we also lose the separation in multiple checks in the GitHub UI which shows what lints failed.

But sure, whatever, go for it, I think these are minor things and we can always revert.

adamjanovsky commented 1 year ago

Nah, gave it a second thought. The idea that you exclude some dependencies from requirements is just ill-based, since these deps are likely to be required by your IDE to run on-the-fly checks. Closing this in favor of #311