Skip to content

feature request: commit list rest endpoint #752

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

Closed
braxton-dameron opened this issue Nov 3, 2023 · 5 comments · Fixed by #798 or #801
Closed

feature request: commit list rest endpoint #752

braxton-dameron opened this issue Nov 3, 2023 · 5 comments · Fixed by #798 or #801

Comments

@braxton-dameron
Copy link

We use this action on pull_request and push, but we've noticed a small discrepancy after merging a couple of our branches. Our PRs that have more than 20 commits do not lint every commit. I've realized this is because of a limit on the push webhook payload.

commits array of objects Required
An array of commit objects describing the pushed commits. (Pushed commits are all commits that are included in the compare between the before commit and the after commit.) The array includes a maximum of 20 commits. If necessary, you can use the Commits API to fetch additional commits. This limit is applied to timeline events only and isn't applied to webhook deliveries.

https://docs.github.com/en/webhooks/webhook-events-and-payloads#push

Is there interest in supporting the REST endpoint to enable consistent linting on large PRs using something like octokit.rest.repos.listCommits(...)? It's atypical for my team to have large PRs like this, but with very active repos and merges it may become more frequent.

https://docs.github.com/en/free-pro-team@latest/rest/commits/commits?apiVersion=2022-11-28#list-commits
https://octokit.github.io/rest.js/v20#repos

@wagoid
Copy link
Owner

wagoid commented Aug 21, 2024

I had to revert the previous fix to overcome this issue #799. I'm thinking here now, in your case the pull_request runs are linting all commits of the PR, right? Is this actually an issue in the end?

I'm thinking the only issue here would be on force pushes that have more than 20 commits on them, but it feels like an edge case that is safely covered by the pull_request run. What do you think?

@braxton-dameron
Copy link
Author

I had to revert the previous fix to overcome this issue #799. I'm thinking here now, in your case the pull_request runs are linting all commits of the PR, right? Is this actually an issue in the end?

I'm thinking the only issue here would be on force pushes that have more than 20 commits on them, but it feels like an edge case that is safely covered by the pull_request run. What do you think?

@wagoid Ah sorry the change we were implementing caused that issue. Yes on pull_request runs we can lint all commits of the PR. It is still an issue when we run the linting process again on push after the PR is merged with more than 20 commits on it. It gives us a discrepancy between the semantic release detected before and after merge. It is an edge case, but at scale with the number of repos and engineers we have consuming the action it is noticeable.

@Brian-Triplett
Copy link
Contributor

In this case we're using semantic release on push and looking at the output of this action to determine what to do with the version number on the trunk. I think this is similar to how you're using commit-and-tag-version.

The not-to-rare case we see is when a PR is merged with >20 commits it's possible the semantic commits are missed and the versioning with semantic release is wrong. What I was hoping to accomplish with #798 was to analyze all the commits from the last merge to trunk to the one incoming on the history from the latest PR.

Issue 799 said all history and I thought this would prevent that but I could be wrong.

sha: before

The behavior I was going for was that push would analyze the same number of commits as the incoming PR (i.e., PR has 32 commits, the last 32 commits are included in the push event).

@wagoid
Copy link
Owner

wagoid commented Aug 22, 2024

Got it! @Brian-Triplett what about adding the same feature you added before, but through a parameter?
We can create a new optional param in the action, called sha for example. Then your different logic would kick in only when this param is provided.
In your release workflow, you would be able to pass the input sha: ${{ github.sha }}

@Brian-Triplett
Copy link
Contributor

@wagoid I was just plain wrong on my assumption of how before worked on listCommits. I tested it out locally and it just grabs commits starting with before and going backwards.

I found what I think is a better solution and posted it in #801

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants