Add GraphQL mutation for configuring code host rate limits#56150
Add GraphQL mutation for configuring code host rate limits#56150varsanojidan wants to merge 4 commits intomainfrom
Conversation
| """ | ||
| The time interval at which the gitQuota's worth of Git requests are replenished. | ||
| """ | ||
| gitReplenishmentIntervalSeconds: Int! |
There was a problem hiding this comment.
I made all of the rate limit configs required for the update because this will be used by a UI that would update all of the rate limits for a code host at the same time, and translates to one CodeHosts.Update db call regardless of how many of the code host's rate limits are actually getting updated. Additionally limits like apiQuota and apiReplenishmentIntervalSeconds are related and its better to make updating them more intentional.
Lmk if you think otherwise 😄.
indradhanush
left a comment
indradhanush
left a comment
There was a problem hiding this comment.
LGTM. Left a few minor comments.
| codeHost, err := r.db.CodeHosts().GetByID(ctx, codeHostID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| codeHost.APIRateLimitQuota = &args.Input.APIQuota | ||
| codeHost.APIRateLimitIntervalSeconds = &args.Input.APIReplenishmentIntervalSeconds | ||
| codeHost.GitRateLimitQuota = &args.Input.GitQuota | ||
| codeHost.GitRateLimitIntervalSeconds = &args.Input.GitReplenishmentIntervalSeconds | ||
|
|
||
| err = r.db.CodeHosts().Update(ctx, codeHost) |
There was a problem hiding this comment.
Does this need to be in a transaction?
There was a problem hiding this comment.
I thought about it initially and decided against it because since this is a full update of the codehost you wont end up in weird half-states, the worst would be if the codehost was deleted between the GetByID and Update, which would just make the update fail. But it is probably better to use a transaction here, i'll change it.
4 participants
Closes: https://github.com/sourcegraph/sourcegraph/issues/55278
Add GraphQL mutation for configuring code host rate limits.
Test plan
Added a bunch of tests and also tested manually.