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

fix(commit): resolve 'always_signoff' configuration and '-s' CLI issues #1206

Merged
merged 5 commits into from
Nov 16, 2024

Conversation

AdrianDC
Copy link
Contributor

@AdrianDC AdrianDC commented Aug 13, 2024

Description

The always_signoff configuration or the CLI -s argument fail upon git commit call
due to the passed syntax being git commit -- -s rather than git commit -s.

The -- is needed only on the commitizen CLI, and if no git argument is passed, the -- should not be forced.

signoff mechanic is deprecated, please use cz commit -- -s instead.
fatal: /tmp/...: '/tmp/... is outside repository at '...'

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

  • Warning and successful if -s
  • Warning and successful if -s --
  • No warning and successful if -- -s
  • No warning and successful if -- -s
  • No warning and successful with always_signoff: true
  • No warning and successful if -- with always_signoff: true
  • No warning and successful if -s with always_signoff: true
  • No warning and successful if -s -- with always_signoff: true
  • No warning and successful if -- -s with always_signoff: true

Steps to Test This Pull Request

.cz.yaml:

---
commitizen:

  # commitizen configurations
  always_signoff: true
  • cz c
  • cz c --
  • cz c -s
  • cz c -s --
  • cz c -- s

Additional context

Related to issue #1135 and #1146

Tested with the python:3.12 Docker image:

docker run -i --rm --entrypoint bash python:3.12 <<EOF
{
  git clone -b signoff https://github.com/AdrianDC/commitizen.git /tmp/commitizen
  cd /tmp/commitizen/
  pip install poetry
  poetry install
  ./scripts/format
  ./scripts/test
}
EOF

@AdrianDC AdrianDC changed the title Signoff fix(commit): resolve 'always_signoff' configuration and '-s' CLI issues Aug 13, 2024
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.58%. Comparing base (120d514) to head (e305ef9).
Report is 490 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1206      +/-   ##
==========================================
+ Coverage   97.33%   97.58%   +0.24%     
==========================================
  Files          42       55      +13     
  Lines        2104     2607     +503     
==========================================
+ Hits         2048     2544     +496     
- Misses         56       63       +7     
Flag Coverage Δ
unittests 97.58% <100.00%> (+0.24%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

Overall, looks good! just left one minor nitpick

Just noticed we haven't marked -s as deprecated yet. We should probably do that. Would it be possible for you to include that in this PR? Thanks!

commitizen/commands/commit.py Outdated Show resolved Hide resolved
@AdrianDC
Copy link
Contributor Author

Just noticed we haven't marked -s as deprecated yet. We should probably do that. Would it be possible for you to include that in this PR? Thanks!

I'd actually be against deprecating -s, given it's the most used parameter after -m since years in our use cases,
and I'd like to get a good adoption rate for either commitizen's cz c -s or git's git commit -s with my hooks.

Also, the always_signoff configuration I fixed in the first place through this MR requires this source code anyways.


If you're interested in my implementations of pre-commit + commitizen + prepare-commit-msg hooks,
feel free to check : https://radiandevcore.gitlab.io/tools/pre-commit-crocodile/commitizen/#configurations-for-commitizen
and the almost-auto-derived specifications : https://radiandevcore.gitlab.io/tools/pre-commit-crocodile/commits/

Initially meant to centralize the development features of my GitLab and Python related tools,
it grew quickly into an automated adoption tool for pre-commit and my commitizen configurations.

I will then deploy the configurations progressively on our internal projects (simply pre-commit-crocodile --configure)
and my developers will be able to use pre-commit-crocodile --enable easily on a day to day basis.

I'll probably use git commit -s on my side, especially with my automated type(scope): evaluators,
and let developers prefer to use cz commit depending on their personal feelings,
both giving a very similar result + same validation, and the CI/CD job component checks all commits either way.

@Lee-W
Copy link
Member

Lee-W commented Aug 22, 2024

Just noticed we haven't marked -s as deprecated yet. We should probably do that. Would it be possible for you to include that in this PR? Thanks!

I'd actually be against deprecating -s, given it's the most used parameter after -m since years in our use cases, and I'd like to get a good adoption rate for either commitizen's cz c -s or git's git commit -s with my hooks.

Also, the always_signoff configuration I fixed in the first place through this MR requires this source code anyways.

If you're interested in my implementations of pre-commit + commitizen + prepare-commit-msg hooks, feel free to check : https://radiandevcore.gitlab.io/tools/pre-commit-crocodile/commitizen/#configurations-for-commitizen and the almost-auto-derived specifications : https://radiandevcore.gitlab.io/tools/pre-commit-crocodile/commits/

Initially meant to centralize the development features of my GitLab and Python related tools, it grew quickly into an automated adoption tool for pre-commit and my commitizen configurations.

I will then deploy the configurations progressively on our internal projects (simply pre-commit-crocodile --configure) and my developers will be able to use pre-commit-crocodile --enable easily on a day to day basis.

I'll probably use git commit -s on my side, especially with my automated type(scope): evaluators, and let developers prefer to use cz commit depending on their personal feelings, both giving a very similar result + same validation, and the CI/CD job component checks all commits either way.

My thought on whether to keep these "commonly used" arguments is detailed #1217 (comment). let's keep the discussion in one place 🙂


https://radiandevcore.gitlab.io/tools/pre-commit-crocodile/commitizen/ looks cool btw 🤩

@AdrianDC
Copy link
Contributor Author

https://radiandevcore.gitlab.io/tools/pre-commit-crocodile/commitizen/ looks cool btw 🤩

Thanks !

Fine, reworking the commits series tomorrow.

@AdrianDC
Copy link
Contributor Author

Reimplemented as discussed, documentations updated and tests too.

Deprecation commit on top of this MR pushed for v4 on MR #1221.


Understood the test_commit_command_shows_description_when_use_help_option failures I couldn't see locally,
the test is restricted to Python 3.13, but the python:3.13.0rc1 image fails at poetry install for now.

@Lee-W
Copy link
Member

Lee-W commented Sep 5, 2024

Hi @AdrianDC , just a head up. I'm aware of these changes, but I am overwhelmed these days. I'll try my best to take a look ASAP.

@AdrianDC
Copy link
Contributor Author

AdrianDC commented Sep 5, 2024

No worries @Lee-W , same here since 1+ year, only had time to work on my GitLab related tools the last weeks.

For the time being, my developers and pre-commit-crocodile users use the prerelease branch anyways,
until all commits are upstreamed, hence no rush : https://github.com/AdrianDC/commitizen/commits/prerelease/

(Tested the sync fork feature of GitHub, but it creates a merge commit rather than rebase, sad...)

@AdrianDC
Copy link
Contributor Author

AdrianDC commented Nov 1, 2024

Hey ! Little up on this one 😉

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

looks good to me! Once the conflict is resolved, I think we're close to merge

If 'always_signoff' is enabled in configurations, or '-s' is used alone on the CLI,
the following errors arise due to 'git commit' argument failures :

> signoff mechanic is deprecated, please use `cz commit -- -s` instead.
> fatal: /tmp/...: '/tmp/... is outside repository at '...'

Signed-off-by: Adrian DC <[email protected]>
…nd sources

Details: The git sources folder ownership may be detected as dubious if running
         in a container with sources mounted to work on fixes and tests,
         breaking 'test_find_git_project_root' and 'test_get_commits_with_signature'
> commitizen.exceptions.GitCommandError: fatal: detected dubious ownership in repository at '...'
---

Signed-off-by: Adrian DC <[email protected]>
@AdrianDC
Copy link
Contributor Author

Rebased, ready to roll 👍

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

LGTM. I'll keep it for a few days so that others can take a look. Thanks for your help!

@Lee-W Lee-W merged commit 9c891d6 into commitizen-tools:master Nov 16, 2024
21 checks passed
@AdrianDC AdrianDC deleted the signoff branch November 16, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants