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

feat: Make cz bump work on shallow clones #649

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robertschweizer
Copy link
Contributor

Description

Try to git fetch missing tags if working on a shallow clone.

This allows reliably setting up your CI to only fetch the latest changes. E.g. Gitlab CI uses shallow clones with a depth of 20 by default: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77576

Only tag_exists() had to be modified to make this work for cz bump. Other functions are not ready yet, more functions would have to include a git fetch for that. There are also other problems still with Gitlab CI's shallow clones, e.g. #593. But for my use-case of just running cz bump and cz check in Gitlab CI this works nicely.

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 -> The printed warning makes this unnecessary.

Additional context

I was worried about slowing down error reporting in non-CI workflows ("git fetch" calls can sometimes take a while if your connection is bad). To minimize the risk for this, I'm now only fetching the tag for shallow clones and always printing a warning when this happens.

I was hit by this issue while trying out Commitizen in Gitlab CI on a monorepo, where some packages haven't seen a release for >50 commits. It's also large enough for the shallow clone to really make sense in the CI.

This allows reliably setting up your CI to only fetch the latest changes.
E.g. Gitlab CI uses shallow clones with a depth of 20 by default:
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77576

Only `tag_exists()` had to be modified for this. `cz changelog` is not
ready yet, more functions would have to include a `git fetch` for that.

I was worried about slowing down error reporting in non-CI workflows ("git
fetch" calls can sometimes take a while if your connection is bad). To
minimize the risk for this, now only fetching the tag for shallow
clones and always printing a warning when this happens.

I was hit by this issue while trying out Commitizen in Gitlab CI on a
monorepo, where some packages haven't seen a release for >50 commits.
@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Base: 97.92% // Head: 98.10% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (416f68c) compared to base (db42a95).
Patch coverage: 89.18% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   97.92%   98.10%   +0.18%     
==========================================
  Files          35       39       +4     
  Lines        1252     1690     +438     
==========================================
+ Hits         1226     1658     +432     
- Misses         26       32       +6     
Flag Coverage Δ
unittests 98.10% <89.18%> (+0.18%) ⬆️

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

Impacted Files Coverage Δ
commitizen/commands/init.py 87.93% <84.00%> (-3.74%) ⬇️
commitizen/__version__.py 100.00% <100.00%> (ø)
commitizen/commands/changelog.py 98.86% <100.00%> (ø)
commitizen/commands/check.py 100.00% <100.00%> (ø)
commitizen/exceptions.py 100.00% <100.00%> (ø)
commitizen/git.py 98.66% <100.00%> (+0.07%) ⬆️
commitizen/bump.py 100.00% <0.00%> (ø)
commitizen/__init__.py 100.00% <0.00%> (ø)
commitizen/changelog.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@robertschweizer
Copy link
Contributor Author

It doesn't look like the CI tests failed because of my change. Please let me know if that's the case in your opinion.

@Lee-W
Copy link
Member

Lee-W commented Jan 5, 2023

@robertschweizer I just reran the CI and it passed. Will take a deeper look. Thanks!

capsys.readouterr() # Reset capsys fixture
cli.main()
stdout, _stderr = capsys.readouterr()
assert re.search("Could not find tag.*trying to fetch", stdout)
Copy link
Member

Choose a reason for hiding this comment

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

Should we also check whether the function returns correctly after fetching?

if tag in c.out:
return True

# In shallow clones (e.g. set up by Gitlab CI), the previous release tag might not
Copy link
Member

Choose a reason for hiding this comment

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

I'm good with this functionality. But one of the concerns I have is whether this is an unexpected action from the user point of view. I'm thinking of using a flag.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this is more CI configuration, is up to the user, I'm fine with a warning and an explanation with the next steps to the user. But the user should ensure the environment (git) is providing commitizen what's needed, not the other way around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reviews!

I agree with your concerns that auto-fetching missing tags is unexpected from the user point of view. Actually it would be cleanest to me if git tag --list {tag} had a flag to do this fetching for shallow clones automatically. But without auto-fetching missing tags in shallow clones, the only option would be to do a full clone, which can be quite slow for large/old repositories.

I believe both Gitlab CI and Github Actions use shallow clones by default. Because of that, Commitizen should ideally handle shallow clones out-of-the-box without setting an extra flag. Do you have examples which workflows could break if we fetch in shallow clones by default? I'm mostly worried about connection issues/the CI not setting up the Git remote for later fetching.

If you keep not being convinced, should we call the flag --auto-fetch and only add it for the cz bump command for now?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @robertschweizer Sorry for late reply. I like the idea of the flag --auto-fetch. @woile What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, a flag and a setting. We can prompt the user in the init command

@robertschweizer
Copy link
Contributor Author

Thanks a lot for your review and feedback! I'm sorry about this, but I actually don't need this change anymore.

In our fork we actually restrict git log output to the CWD (.) to run commitizen on monorepo folders. It turns out filtering logs by directory does not work with shallow clones unless you unshallow the complete history up to the reference tag. Combine that with Gitlab's unstable behavior when doing a lot of unshallowing made us go back to using full clones for this CI job.

Feel free to close this MR unless someone else finds it useful.

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.

3 participants