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

Conventional Commits pre-commit check is not working #130

Open
srid opened this issue Mar 7, 2024 · 12 comments
Open

Conventional Commits pre-commit check is not working #130

srid opened this issue Mar 7, 2024 · 12 comments
Labels
bug Something isn't working

Comments

@srid
Copy link
Collaborator

srid commented Mar 7, 2024

The second commit in PR #127 has the commit message use same installer - yet the pre-commit check did not catch it, as it doesn't comfort to conventional commits spec.

@srid srid added the bug Something isn't working label Mar 7, 2024
@srid
Copy link
Collaborator Author

srid commented Mar 7, 2024

Commit's vanished because I had to force push, but the commit message was literally use same installer.

@srid srid pinned this issue Mar 7, 2024
@srid
Copy link
Collaborator Author

srid commented Mar 7, 2024

The hook itself works,

image

However, if I skip it:

image

And run the flake check:

image image

... it passes ^

@srid
Copy link
Collaborator Author

srid commented Mar 7, 2024

I don't think the pre-commit module actually adds a flake check.

Also, https://github.com/convco/convco

We can run this manually

image

@terlar
Copy link

terlar commented Mar 7, 2024

Yeah, when running in a flake check it won't have access to .git/COMMIT_EDITMSG and will therefore not be able to check the commit message. The workaround is to use nix run nixpkgs#pre-commit directly on CI. That is what I found as well, although I was using conform.

@srid
Copy link
Collaborator Author

srid commented Mar 8, 2024

A PR can have multiple commits, so whatever flake check we add must check all of them.

@srid
Copy link
Collaborator Author

srid commented Mar 8, 2024

But since git history will be available, perhaps it should be run outside of Nix, in github actions.

@shivaraj-bh
Copy link
Member

shivaraj-bh commented Mar 8, 2024

This command checks for all the commits on the current branch, starting from main:

cz check --rev-range main..HEAD

@srid
Copy link
Collaborator Author

srid commented Mar 8, 2024

Cool. We should also consider #134 when deciding between cz and convco.

@shivaraj-bh
Copy link
Member

This command checks for all the commits on the current branch, starting from main:

cz check --rev-range main..HEAD

Can’t do this until NixOS/nix#6900

@shivaraj-bh
Copy link
Member

I have made some progress at https://github.com/juspay/services-flake/tree/cz-check, will wait until there is a reliable way to copy .git to the store.

@srid
Copy link
Collaborator Author

srid commented May 20, 2024

Copying .git to the Nix store sounds like a bad idea in general? We only need history of commit messages, not the whole index (much less copy a variation of it every time to the store).

@shivaraj-bh
Copy link
Member

We only need history of commit messages, not the whole index

I doubt we can pass this to flake check in a straightforward way.

One hack to make this work will be to use writeShellApplication and run cz check --rev-range main..HEAD via nix run.

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

No branches or pull requests

3 participants