sentry-cli icon indicating copy to clipboard operation
sentry-cli copied to clipboard

Limit commit count from previous commit.

Open ankitghosh opened this issue 1 year ago • 4 comments

Steps to Reproduce

Recently our build started failing for uploading commit history to a build. So why it failed because we merged bunch of commits which were huge in number. So we were getting 413 error from get_commits_from_git function.

Expected Result

Ideally commit should have been truncated with initial-depth value which we were passing.

Actual Result

It failed with 413 error.

Logs

I raise a PR as well for the fix https://github.com/getsentry/sentry-cli/pull/2006. Basically I am adding default_count in get_commits_from_git function

ankitghosh avatar Apr 10 '24 11:04 ankitghosh

@ankitghosh, can you please provide the complete log output from running the CLI? If possible, please run the CLI with the --log-level=debug option, so that the full debug output is included.

I raise a PR as well for the fix https://github.com/getsentry/sentry-cli/pull/2006. Basically I am adding default_count in get_commits_from_git function

As I already mentioned in the comment I left on #2006 when I closed the PR, this problem requires a different solution from what you suggest in #2006. Likely, we will need to introduce a separate option to limit the number of commits we consider for creating a new release.

szokeasaurusrex avatar Apr 11 '24 11:04 szokeasaurusrex

@ankitghosh, can you please provide the complete log output from running the CLI? If possible, please run the CLI with the --log-level=debug option, so that the full debug output is included.

Hey i will try to generate and paste it here but in short it hangs after calling this get_commits_from_git function.

Hmm agreed we could have a new flag, considering initial-depth is used for new release. But i think we should also have a default number which would prevent this from happening for users who does not provide a flag.

ankitghosh avatar Apr 12 '24 11:04 ankitghosh

Hey i will try to generate and paste it here but in short it hangs after calling this get_commits_from_git function.

Thank you in advance!

But i think we should also have a default number which would prevent this from happening for users who does not provide a flag.

I understand your perspective here, but creating a default number is a breaking change, and so we will not be able to add a default number.

For example, consider what would happen if we set the default number of commits for a release to 20, as you suggested in #2006. Let's say someone wants to create a new release, but they have 30 commits since the last release. Currently, this would work fine – the new release would be created with all 30 commits. However, with #2006, only the 20 most recent commits are included, even though the user probably would have intended for all 30 to be included.

Adding a new flag should solve your problem, and we can also consider adding an error message for this command so that we inform users who encounter the same issue you did that they can use the flag to work around the problem.

szokeasaurusrex avatar Apr 12 '24 13:04 szokeasaurusrex

Yup fair point 👍 a new flag makes sense with error message in place.

ankitghosh avatar Apr 12 '24 13:04 ankitghosh

@ankitghosh are you able to work around this issue by manually setting the commits you wish to include in the release with the -c option? If yes, then I would suggest just using that option, and closing this issue

szokeasaurusrex avatar Dec 10 '24 16:12 szokeasaurusrex

Yes was able to do that, hence closing it

ankitghosh avatar Dec 10 '24 16:12 ankitghosh