Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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

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

Merged
Lee-W merged 5 commits into commitizen-tools:master from AdrianDC:signoff
Nov 16, 2024

Conversation

Copy link
Contributor

@AdrianDC AdrianDC commented Aug 13, 2024 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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!

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.

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 reacted with thumbs up emoji

Copy link
Contributor Author

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

Thanks !

Fine, reworking the commits series tomorrow.
Lee-W reacted with heart emoji

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.

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.

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

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 <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>
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
Reviewers

@Lee-W Lee-W Lee-W approved these changes

@woile woile Awaiting requested review from woile woile is a code owner

@noirbizarre noirbizarre Awaiting requested review from noirbizarre noirbizarre is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants

AltStyle によって変換されたページ (->オリジナル) /