Skip to content

Correct the runIDFromURLRE regex to properly match the callbackURL - #3495

Merged
gmlewis merged 4 commits into
google:masterfrom
pputman-clabs:pputman-clabs/fix-getrunid-helper-regex
Feb 26, 2025
Merged

Correct the runIDFromURLRE regex to properly match the callbackURL#3495
gmlewis merged 4 commits into
google:masterfrom
pputman-clabs:pputman-clabs/fix-getrunid-helper-regex

Conversation

@pputman-clabs

Copy link
Copy Markdown
Contributor

the callbackURL from the event previously didn't match with the regex, and was always causing a "no match found error". This includes the full URL and should match.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I should have caught this before (sorry about that), but this repo supports both public GitHub and enterprise GitHub installations, which means that we can't hard-code a domain name in a regex.

Instead of your change, we should simply remove the leading carat (^) and then it should support both styles of URLs.

Please update this PR and then add a test for both styles of URLs showing that it should work on both. Thank you, @pputman-clabs.

@gmlewisgmlewis changed the title correct the regex to properly match the callbackURLCorrect the regex to properly match the callbackURLFeb 25, 2025
@gmlewisgmlewis changed the title Correct the regex to properly match the callbackURLCorrect the runIDFromURLRE regex to properly match the callbackURLFeb 25, 2025
@codecov

codecovBot commented Feb 25, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.03%. Comparing base (a25b53e) to head (2cb7af7).
Report is 1 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #3495 +/- ##
=======================================
Coverage 91.03% 91.03% =======================================
Files 179 179 Lines 15574 15574 =======================================
Hits 14178 14178 Misses 1223 1223 Partials 173 173 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…move the carat(^) to match any url, along with tests for both https://api.github.com along with a dummy url for enterprise Github

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One more tweak, please, @pputman-clabs, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

Comment threadgithub/github_test.go Outdated
@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Feb 25, 2025

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @pputman-clabs!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis

Copy link
Copy Markdown
Collaborator

@stevehipwell - might you have time for a code review? Thank you!

@stevehipwellstevehipwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Feb 26, 2025
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @stevehipwell!
Merging.

@gmlewis
gmlewis merged commit e9d8a58 into google:masterFeb 26, 2025
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto 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.

3 participants

@pputman-clabs@gmlewis@stevehipwell