Add fields to RateLimits struct #2340

shokada · 2022-04-24T00:37:10Z

What this PR does / why we need it:

I added fields to RateLimits struct to get rate limits other than core and search

Special notes for your reviewer:

Since there is no documentation for source_import, code_scanning_upload, actions_runner_registration, and scim, I did not know which endpoints had the rate limit set, so I did not fix category function.

go-github/github/github.go

Lines 1126 to 1134 in 942d33d

    
           // category returns the rate limit category of the endpoint, determined by Request.URL.Path. 
        
           func category(path string) rateLimitCategory { 
        
           	switch { 
        
           	default: 
        
           		return coreCategory 
        
           	case strings.HasPrefix(path, "/search/"): 
        
           		return searchCategory 
        
           	} 
        
           }

google-cla · 2022-04-24T00:37:16Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

github/github.go

shokada · 2022-04-24T00:44:12Z

github/github.go

+	SourceImport              *Rate `json:"source_import"`
+	CodeScanningUpload        *Rate `json:"code_scanning_upload"`
+	ActionsRunnerRegistration *Rate `json:"actions_runner_registration"`
+	Scim                      *Rate `json:"scim"`


There seems to be no document for source_import, code_scanning_upload, actions_runner_registration, scim.

gmlewis

Thank you, @shokada!
Let's tweak some of the acronym names, please.
Then we should be ready for a second LGTM+Approval before merging.

github/github.go

gmlewis · 2022-04-24T18:54:25Z

Also please remember to run gofmt on your edited files and go generate ./... , then push (not force-push) the changed files to the PR.

shokada · 2022-04-24T22:49:55Z

Run go generate ./... 5887f39

codecov · 2022-04-24T22:53:19Z

Codecov Report

Merging #2340 (a6c7bcd) into master (00e4233) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2340   +/-   ##
=======================================
  Coverage   98.04%   98.05%           
=======================================
  Files         118      118           
  Lines       10458    10476   +18     
=======================================
+ Hits        10254    10272   +18     
  Misses        140      140           
  Partials       64       64

Impacted Files	Coverage Δ
github/github.go	`97.80% <100.00%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00e4233...a6c7bcd. Read the comment docs.

shokada · 2022-04-24T23:31:23Z

github/github.go

+	sourceImportCategory              //nolint:deadcode,varcheck
+	codeScanningUploadCategory        //nolint:deadcode,varcheck
+	actionsRunnerRegistrationCategory //nolint:deadcode,varcheck
+	scimCategory                      //nolint:deadcode,varcheck


Since TestClient_rateLimits does not pass, I have defined unused constants.

Instead of doing this, we actually need to use these values in the category function (lines 1127-1134 below) and in the RateLimits method (lines 1154-1163 below).

in the RateLimits method (lines 1154-1163 below).

fixed a6c7bcd

in the category function (lines 1127-1134 below)

I could not figure out how to fix the category function for the following reasons...

Since there is no documentation for source_import, code_scanning_upload, actions_runner_registration, and scim, I did not know which endpoints had the rate limit set, so I did not fix category function.
#2340 (comment)

gmlewis · 2022-04-25T11:31:48Z

github/github.go

+	sourceImportCategory              //nolint:deadcode,varcheck
+	codeScanningUploadCategory        //nolint:deadcode,varcheck
+	actionsRunnerRegistrationCategory //nolint:deadcode,varcheck
+	scimCategory                      //nolint:deadcode,varcheck


Instead of doing this, we actually need to use these values in the category function (lines 1127-1134 below) and in the RateLimits method (lines 1154-1163 below).

gmlewis

Thank you, @shokada !
LGTM.

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

raynigon

LGTM

gmlewis · 2022-05-16T14:16:01Z

LGTM

Thank you, @raynigon !
Merging.

patrickmarabeas · 2022-05-25T01:49:44Z

@gmlewis is it possible to get a fresh release pushed out? Thanks!

gmlewis · 2022-05-25T02:02:36Z

Yes, I can work on one tomorrow.

gmlewis · 2022-05-25T14:53:00Z

@gmlewis is it possible to get a fresh release pushed out? Thanks!

@patrickmarabeas - v45 has now been released: https://github.com/google/go-github/releases/tag/v45.0.0

Add fields to RateLimits struct.

942d33d

shokada commented Apr 24, 2022

View reviewed changes

github/github.go Show resolved Hide resolved

shokada commented Apr 24, 2022

View reviewed changes

github/github.go Show resolved Hide resolved

shokada commented Apr 24, 2022

View reviewed changes

shokada marked this pull request as ready for review April 24, 2022 01:06

gmlewis changed the title ~~Add fields to RateLimits struct.~~ Add fields to RateLimits struct Apr 24, 2022

gmlewis requested changes Apr 24, 2022

View reviewed changes

github/github.go Outdated Show resolved Hide resolved

github/github.go Outdated Show resolved Hide resolved

gmlewis added the NeedsReview PR is awaiting a review before merging. label Apr 24, 2022

shokada added 2 commits April 25, 2022 07:44

fix name

8b2ba54

run go generate ./...

5887f39

shokada requested a review from gmlewis April 24, 2022 22:51

Add nolint comment

86f3ae6

shokada commented Apr 24, 2022

View reviewed changes

gmlewis requested changes Apr 25, 2022

View reviewed changes

Add values to RateLimits method

a6c7bcd

shokada requested a review from gmlewis April 25, 2022 12:18

gmlewis approved these changes Apr 25, 2022

View reviewed changes

raynigon approved these changes May 16, 2022

View reviewed changes

gmlewis removed the NeedsReview PR is awaiting a review before merging. label May 16, 2022

gmlewis merged commit a58d5f0 into google:master May 16, 2022

gmlewis mentioned this pull request May 25, 2022

Bump version of go-github library to v45 #2374

Merged

gal-legit mentioned this pull request Jan 18, 2023

Rate limit does not cover all categories & secondary rate limit not preventing requests #2633

Closed

	// category returns the rate limit category of the endpoint, determined by Request.URL.Path.
	func category(path string) rateLimitCategory {
	switch {
	default:
	return coreCategory
	case strings.HasPrefix(path, "/search/"):
	return searchCategory
	}
	}

Add fields to RateLimits struct #2340

Add fields to RateLimits struct #2340

Uh oh!

Conversation

shokada commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Special notes for your reviewer:

Uh oh!

google-cla bot commented Apr 24, 2022

Uh oh!

Uh oh!

Uh oh!

shokada Apr 24, 2022

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gmlewis commented Apr 24, 2022

Uh oh!

shokada commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shokada Apr 24, 2022

Choose a reason for hiding this comment

Uh oh!

gmlewis Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

shokada Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

gmlewis Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

raynigon left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented May 16, 2022

Uh oh!

patrickmarabeas commented May 25, 2022

Uh oh!

gmlewis commented May 25, 2022

Uh oh!

gmlewis commented May 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shokada commented Apr 24, 2022 •

edited

Loading

shokada commented Apr 24, 2022 •

edited

Loading

codecov bot commented Apr 24, 2022 •

edited

Loading