intel / p3-analysis-library

A library simplifying the collection and interpretation of P3 data.
https://intel.github.io/p3-analysis-library/
MIT License
7 stars 10 forks source link

CI and pre-commit alignment #32

Closed laserkelvin closed 5 months ago

laserkelvin commented 5 months ago

Related issues

Not really an issue persay, but was brought about by errors in the existing workflow by #18.

Proposed changes

List out—with high level descriptions—what the commits within this PR do:

laserkelvin commented 5 months ago

@Pennycook the unit tests in P3 are pretty light weight and we could consider even having them run with pre-commit. What do you think?

Also this PR doesn't hit any Python code to trigger all of the formatting, etc. I can push a commit that deliberately tests this once you've approved the intended behavior based on current configs.

laserkelvin commented 5 months ago

False positive picked stuff up that it should, but one that was not intended:


bandit...................................................................Failed
- hook id: bandit
- exit code: 2

[config]    ERROR   expected '<document start>', but found '<scalar>'
  in ".bandit", line 2, column 1
[main]  ERROR   .bandit : Error parsing file.

Bandit configuration issue?

Pennycook commented 5 months ago

Bandit configuration issue?

Argh. Yes, I forgot about this. We encountered this with CBI as well: https://github.com/intel/code-base-investigator/commit/07339618bc052be25c0877c788840aa49384626f. flake8-bandit expects a TOML file called .bandit, while bandit itself expects a configuration file in a different format.

I think you can just copy across the .bandit file from CBI. It's probably a good idea to do that anyway, since it explicitly lists out a bunch of stuff we don't want.

Although, having said that, I don't know if the way the pre-commit hook is trying to invoke bandit will play nicely with whatever pyproject.toml is trying to do.

Sorry, this is proving to be more complicated than I expected...

laserkelvin commented 5 months ago

Fixed in 64aad2e.

Not really a problem persay, but according to the bandit docs .bandit is nominally expected to be INI syntax, but CBI's .bandit contents are actually YAML which I've copied over. It seems to be perfectly happy though, so I'm not really fussed about it but worth fixing later on if it ever gets confusing.

laserkelvin commented 5 months ago

All checks pass @Pennycook! I think it's good to merge on my end.