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

CI and pre-commit alignment #32

Merged
merged 18 commits into from
Apr 3, 2024

Conversation

laserkelvin
Copy link
Contributor

@laserkelvin laserkelvin commented Apr 3, 2024

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:

  • Updates the pre-commit configuration to match what was defined for CBI, but with small modifications to match P3
  • Removes redundant dev dependencies and configurations from pyproject.toml that are spec'd out in .pre-commit.yaml
  • Refactors the existing Github "lint-format" action to use the pre-commit workflow directly
  • Bumps the minimum version of Python supported to 3.9 (closes [Feature request]: Python 3.8 EOL in 2024 #23)

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
@laserkelvin laserkelvin added the bug Something isn't working label Apr 3, 2024
pre-commit configuration handles the file excludes, and we should be running pre-commit hooks on every file that we should.

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
@laserkelvin
Copy link
Contributor Author

laserkelvin commented Apr 3, 2024

@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.

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
.github/workflows/run-precommit.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
laserkelvin and others added 4 commits April 3, 2024 09:52
Co-authored-by: John Pennycook <john.pennycook@intel.com>
Signed-off-by: Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
@laserkelvin
Copy link
Contributor Author

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?

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
@Pennycook
Copy link
Contributor

Bandit configuration issue?

Argh. Yes, I forgot about this. We encountered this with CBI as well: intel/code-base-investigator@0733961. 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
Copy link
Contributor Author

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.

Signed-off-by: Kin Long Kelvin Lee <kin.long.kelvin.lee@intel.com>
@laserkelvin
Copy link
Contributor Author

laserkelvin commented Apr 3, 2024

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

Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

LGTM. Hopefully this fixes the issue @swright87 is having... 🤞

@Pennycook Pennycook merged commit 7b929d2 into intel:main Apr 3, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request]: Python 3.8 EOL in 2024
2 participants