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

Go Github Bypass Unexported #3030

Open
nkreiger opened this issue Dec 14, 2023 · 5 comments
Open

Go Github Bypass Unexported #3030

nkreiger opened this issue Dec 14, 2023 · 5 comments

Comments

@nkreiger
Copy link

bypassRateLimitCheck requestContext = iota

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

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 14, 2023

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.

@nkreiger
Copy link
Author

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

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 14, 2023

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.

@nkreiger
Copy link
Author

@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

// checkRateLimitBeforeDo does not make any network calls, but uses existing knowledge from

I do not have the scope to confirm this locally FYI

@gmlewis
Copy link
Collaborator

gmlewis commented Dec 14, 2023

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.

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

No branches or pull requests

2 participants