go-github icon indicating copy to clipboard operation
go-github copied to clipboard

Go Github Bypass Unexported

Open nkreiger opened this issue 2 years ago • 5 comments

https://github.com/google/go-github/blob/c4ec3276de8584568e12e2a6af0d84a29a5ba8d8/github/github.go#L795

Is there a reason this is unexported? I'm experimenting with the go-github ratelimiter, and it seems like it doesn't even reach the RoundTrip function to properly sleep with BareDo because its first getting blocked by the Go Github Client Check, which I can't bypass because the context is unexported

nkreiger avatar Dec 14 '23 19:12 nkreiger

This was implemented in #1907.

Can you give an example of how you would use it if it were exported?

Alternatively, feel free to write up a PR showing your use case as a new unit test and we can take a look at it.

gmlewis avatar Dec 14 '23 19:12 gmlewis

@gmlewis thanks for the qr, let me validate because it may have been the github changes 3 days ago switching the message

nkreiger avatar Dec 14 '23 19:12 nkreiger

OK, cool.

Also, I should have mentioned that the original problem that was trying to be solved was #1899 which would be great if you could take a look at when you have a moment.

gmlewis avatar Dec 14 '23 19:12 gmlewis

@gmlewis took a look -- I do think the problem remains with it not being exported.

Based on review of the code, the github client will not make the extra call if the primary rate limit has been exceeded. Therefore, any RoundTrip rate limiting logic defined in the go-github rate limiter, or any other custom client, will NOT be reached.

It would be awesome to expose this, I'm happy too.

LMK what you think

https://github.com/google/go-github/blob/75644ea48541a9c99ad73bf86e50bae667b10a20/github/github.go#L631

I do not have the scope to confirm this locally FYI

nkreiger avatar Dec 14 '23 21:12 nkreiger

So the gist you pointed me to appears to be extremely old and doesn't even contain checkSecondaryRateLimitBeforeDo. There also appears to be no difference to checkRateLimitBeforeDo, so I'm not sure what I'm looking at.

If you want to make a PR based on the current top-of-trunk, that would be fine, and it should be much easier to compare.

gmlewis avatar Dec 14 '23 21:12 gmlewis