-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1206 +/- ##
==========================================
+ Coverage 97.33% 97.61% +0.27%
==========================================
Files 42 55 +13
Lines 2104 2518 +414
==========================================
+ Hits 2048 2458 +410
- Misses 56 60 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
db79133
to
f903f01
Compare
There was a problem hiding this 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!
I'd actually be against deprecating Also, the If you're interested in my implementations of pre-commit + commitizen + prepare-commit-msg hooks, Initially meant to centralize the development features of my GitLab and Python related tools, I will then deploy the configurations progressively on our internal projects (simply I'll probably use |
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 🤩 |
Thanks ! Fine, reworking the commits series tomorrow. |
2742424
to
d211348
Compare
Reimplemented as discussed, documentations updated and tests too. Deprecation commit on top of this MR pushed for v4 on MR #1221. Understood the |
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. |
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 <radian.dc@gmail.com>
Signed-off-by: Adrian DC <radian.dc@gmail.com>
Signed-off-by: Adrian DC <radian.dc@gmail.com>
Signed-off-by: Adrian DC <radian.dc@gmail.com>
…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 <radian.dc@gmail.com>
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 (Tested the sync fork feature of GitHub, but it creates a merge commit rather than rebase, sad...) |
Description
The
always_signoff
configuration or the CLI-s
argument fail upongit commit
calldue to the passed syntax being
git commit -- -s
rather thangit commit -s
.The
--
is needed only on the commitizen CLI, and if no git argument is passed, the--
should not be forced.Checklist
./scripts/format
and./scripts/test
locally to ensure this change passes linter check and testExpected behavior
-s
-s --
-- -s
-- -s
always_signoff: true
--
withalways_signoff: true
-s
withalways_signoff: true
-s --
withalways_signoff: true
-- -s
withalways_signoff: true
Steps to Test This Pull Request
.cz.yaml:
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: