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

Increase number of commits checked for squash merge #866

Conversation

tevanoff
Copy link
Contributor

@tevanoff tevanoff commented Nov 30, 2023

When a Chromatic build is run on a squashed merge commit, there's no way to determine the source branch to look for ancestor builds since the git history is lost during the squash. To get around this, we try to query the git provider for a PR that resulted in the squashed commit, which will give us the information we need.

We're currently only doing this lookup on the commit that a build is running on. This change increases the number of parent commits that we try to do the lookup on.

📦 Published PR as canary version: 10.0.1--canary.866.7107771516.0

✨ Test out this PR locally via:

npm install [email protected]
# or 
yarn add [email protected]

@tevanoff tevanoff force-pushed the tom/ap-3123-squashrebase-merge-detection-can-fail-if-build-hasnt-run-for branch 2 times, most recently from 590191d to c3c253b Compare December 2, 2023 01:04
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

This is great, a couple tweaks needed I think.

node-src/git/getParentCommits.test.ts Outdated Show resolved Hide resolved
node-src/git/getParentCommits.test.ts Outdated Show resolved Hide resolved
node-src/git/getParentCommits.ts Show resolved Hide resolved
@tmeasday
Copy link
Member

tmeasday commented Dec 5, 2023

@tevanoff I tested out the canary and it worked well. In fact better than I expected:

This PR's squash merge (no build, this commit) was detected on a build for a later commit on the base branch

But even better, it didn't use a build on the head branch I created after the merge.

I guess PullRequest.headBranch already checks that builds aren't committed after the merge:

https://github.com/chromaui/chromatic/blob/e930b99542e9b0895179aa377a9de6cb6805552f/services/index/model/PullRequest.js#L113-L120

@tevanoff tevanoff force-pushed the tom/ap-3123-squashrebase-merge-detection-can-fail-if-build-hasnt-run-for branch from a7e171c to 847d236 Compare December 5, 2023 18:09
tmeasday and others added 10 commits December 5, 2023 12:33
Instead of first checking if the current commit is a PR merge commit, instead, calculate all possible ancestor commits, THEN append the PR head to that list if relevant.

This is a first step towards checking more than one commit being a PR merge commit
@tevanoff tevanoff force-pushed the tom/ap-3123-squashrebase-merge-detection-can-fail-if-build-hasnt-run-for branch from 847d236 to b9cf522 Compare December 5, 2023 20:33
@tevanoff tevanoff marked this pull request as ready for review December 5, 2023 20:33
@tevanoff
Copy link
Contributor Author

tevanoff commented Dec 5, 2023

@tevanoff I tested out the canary and it worked well. In fact better than I expected:

Thanks @tmeasday, that's great to hear! I've had good luck with testing too. 🎉

I guess PullRequest.headBranch already checks that builds aren't committed after the merge:

https://github.com/chromaui/chromatic/blob/e930b99542e9b0895179aa377a9de6cb6805552f/services/index/model/PullRequest.js#L113-L120

Oh nice find, that's good to know!

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM! One final comment

node-src/git/getParentCommits.test.ts Outdated Show resolved Hide resolved
@tevanoff tevanoff added minor Auto: Increment the minor version when merged release Auto: Create a `latest` release when merged labels Dec 6, 2023
@tevanoff tevanoff added this pull request to the merge queue Dec 7, 2023
Merged via the queue into main with commit 4d890d1 Dec 7, 2023
19 of 21 checks passed
@tevanoff tevanoff deleted the tom/ap-3123-squashrebase-merge-detection-can-fail-if-build-hasnt-run-for branch December 7, 2023 17:11
@ghengeveld
Copy link
Member

🚀 PR was released in v10.1.0 🚀

@ghengeveld ghengeveld added the released Verdict: This issue/pull request has been released label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Auto: Increment the minor version when merged release Auto: Create a `latest` release when merged released Verdict: This issue/pull request has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants