Skip to content

DEVPROD-26260 Call GitHub Compare API with SHAs instead of labels#9776

Merged
ZackarySantana merged 20 commits intoevergreen-ci:mainfrom
ZackarySantana:DEVPROD-26260
Feb 10, 2026
Merged

DEVPROD-26260 Call GitHub Compare API with SHAs instead of labels#9776
ZackarySantana merged 20 commits intoevergreen-ci:mainfrom
ZackarySantana:DEVPROD-26260

Conversation

@ZackarySantana
Copy link
Contributor

@ZackarySantana ZackarySantana commented Feb 6, 2026

DEVPROD-26260

Description

This uses the PR ref rather than label when we use the github compare API with PRs.

Testing

I tested this locally and on staging. I made three different PRs and retried after deploying.

  1. I pushed a change, got a status check, merged main on the GitHub UI, got a new status check.
  2. I pushed a change, got a status check, merged main via the CLI, got a new status check.
  3. I pushed a change that already merged main and got one status check.

For all three:

I also printed out the difference between the ref and sha here in case anyone was interested.

For example, this PR.

Before
Screenshot 2026-02-06 at 11 00 52 AM

After
Screenshot 2026-02-06 at 11 01 04 AM

Notice how the github pr commit didn't change (same head commit), but the merge base did.

@ZackarySantana ZackarySantana self-assigned this Feb 6, 2026
@ZackarySantana ZackarySantana requested a review from a team February 6, 2026 17:25
@ZackarySantana ZackarySantana changed the title DEVPROD-26260 Call GitHub Compare API with PR ref instead of label DEVPROD-26260 Call GitHub Compare API with SHAs instead of labels Feb 9, 2026
@@ -1,412 +1,425 @@
{
"action": "opened",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced the PR in this test file because our github app doesn't have access to baxterthehacker/public-repo (nor should it). This is also more future proof (external repo can go private, renamed, or deleted)

Copy link
Contributor

@Kimchelly Kimchelly left a comment

Choose a reason for hiding this comment

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

Nice find!

s.Equal(expectedMergeBase, hash)
})

// This test should fail, but it triggers the retry logic which in turn
Copy link
Contributor

Choose a reason for hiding this comment

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

But it could succeed one day if we make a conifer repo! 🌲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very true. TBH this test isn't even testing anything? It was added 7 years ago, and is getting stopped by trying to get an installation token on an undefined repo. I was thinking of just removing it, wdyt?

@ZackarySantana ZackarySantana merged commit 2416fa5 into evergreen-ci:main Feb 10, 2026
12 checks passed
@ZackarySantana ZackarySantana deleted the DEVPROD-26260 branch February 10, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants