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(pre-commit): add gitlint #1573

Merged
merged 1 commit into from Feb 22, 2022
Merged

ci(pre-commit): add gitlint #1573

merged 1 commit into from Feb 22, 2022

Conversation

BreadGenie
Copy link
Contributor

@BreadGenie
Copy link
Contributor Author

BreadGenie commented Feb 9, 2022

Running of gitlint in CI is not via pre-commit (due to the hook type being commit-msg). I've used a workaround to get it working in our CI.

.gitlint Outdated Show resolved Hide resolved
Copy link
Contributor

@Molkree Molkree left a comment

Choose a reason for hiding this comment

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

git log -1 --pretty=%B | gitlint`

This will only check the last commit, right? What if someone pushes more at once? We need to run on the range of commits in PR.

I feel like the effort/usefulness ratio of properly setting up a CI for this is not great. Every new contrib is gonna trigger this check :)

.github/workflows/linting.yml Outdated Show resolved Hide resolved
.github/workflows/linting.yml Outdated Show resolved Hide resolved
.github/workflows/linting.yml Outdated Show resolved Hide resolved
.github/workflows/linting.yml Outdated Show resolved Hide resolved
.github/workflows/linting.yml Outdated Show resolved Hide resolved
.gitlint Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2022

Codecov Report

Merging #1573 (80f4e51) into main (62ae407) will decrease coverage by 0.04%.
The diff coverage is 53.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1573      +/-   ##
==========================================
- Coverage   80.36%   80.31%   -0.05%     
==========================================
  Files         281      281              
  Lines        5581     5588       +7     
  Branches      913      916       +3     
==========================================
+ Hits         4485     4488       +3     
- Misses        897      899       +2     
- Partials      199      201       +2     
Flag Coverage Δ
longtests 80.31% <53.84%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cve_bin_tool/cli.py 70.79% <33.33%> (-0.64%) ⬇️
cve_bin_tool/version_scanner.py 56.86% <50.00%> (+0.42%) ⬆️
test/test_checkers.py 93.54% <66.66%> (-3.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d95852...80f4e51. Read the comment docs.

@BreadGenie
Copy link
Contributor Author

BreadGenie commented Feb 10, 2022

The command for gitlint seems to be not working in our CI. gitlint is using git rev-list <--commits' argument> under the hood and it seems like git can't detect the main or current feature branches. Tried the same with range of commits.

@BreadGenie
Copy link
Contributor Author

The same code works on my fork (failing conventional commit check and passing check) but not here 🤔. Weird.

@Molkree
Copy link
Contributor

Molkree commented Feb 10, 2022

@BreadGenie, what you did doesn't work because this repo doesn't have the new branch so you can't just do origin\branch-from-fork. You were pretty close in your attempts, you have used exact commits before but it didn't work because checkout Action only gets the last commit by default, then you changed it to checkout all commits but at the same time changed commit range to use (wrong) refs instead (and also modified what commit to checkout later).

So what you need to do is checkout more commits (can just leave 0 to checkout all history but maybe 50/100 would be enough) and use exact commits for the range.

Look at the last commit here #1578

@BreadGenie
Copy link
Contributor Author

Thanks a lot for the explanation. Good to know that I was closer to what we intended to do. :D

.gitlint Outdated Show resolved Hide resolved
@terriko
Copy link
Contributor

terriko commented Feb 16, 2022

Is this working as expected and ready to merge? I can't quite tell from the comments what state we're in.

@BreadGenie
Copy link
Contributor Author

BreadGenie commented Feb 18, 2022

The commit message test for 3e9b00d failed. So it is working as intended.

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

Okay, let's give this a shot then! Thanks for all the investigation @BreadGenie and @Molkree

@terriko terriko merged commit 184b4e8 into intel:main Feb 22, 2022
@BreadGenie BreadGenie deleted the gitlint branch February 22, 2022 23:32
terriko pushed a commit to terriko/cve-bin-tool that referenced this pull request Mar 9, 2022
Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>

Co-authored-by: Dmitry Volodin <mr.molkree@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci: Enforce conventional commit message format in commits
4 participants