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

fix: use RateLimitBackoff client in LD API #437

Merged
merged 3 commits into from Mar 14, 2024

Conversation

jonathan-ostrander
Copy link
Contributor

#434 attempted to handle the rate limit from the LD API but the LD API client uses http.DefaultClient by default. This change passes in the client with RateLimitBackoff set into the call to init the LD API client.

launchdarkly#434 attempted to handle the rate limit from the LD API but the LD API client uses `http.DefaultClient` by default. This change passes in the client with `RateLimitBackoff` set into the call to init the LD API client.
@jonathan-ostrander jonathan-ostrander requested a review from a team as a code owner March 12, 2024 14:34
@jonathan-ostrander
Copy link
Contributor Author

@jazanne sorry for needing a follow-up PR, but the original PR did not actually affect any of the LD API calls. This should have the intended behavior with regards to the API rate limits.

@jazanne
Copy link
Contributor

jazanne commented Mar 14, 2024

@jonathan-ostrander thanks for raising this issue. We've decided to remove the use of that secondary api client here. Hope to have a release out by end of week. Thank you for your patience

@jonathan-ostrander
Copy link
Contributor Author

@jazanne could we consider this a hot fix for the current build while that PR gets through review? I'm worried that such a large PR might take longer than the next couple of days to be merged.

@jazanne
Copy link
Contributor

jazanne commented Mar 14, 2024

@jonathan-ostrander it looks like tests are failing with nil pointer deference related to the log line you added

Failed
panic: runtime error: invalid memory address or nil pointer dereference
/usr/local/go/src/testing/testing.go:1526 +0x372
/usr/local/go/src/testing/testing.go:1529 +0x650
/usr/local/go/src/runtime/panic.go:890 +0x263
/home/circleci/project/internal/ld/ld.go:88 +0x2dc
/home/circleci/project/internal/ld/ld_test.go:272 +0x2d2

If we get a fix for that I can try getting a hotfix out today

@jonathan-ostrander
Copy link
Contributor Author

Whoops, forgot to add the Request to the test fixture. Fixed in ff79d49.

@jazanne jazanne enabled auto-merge (squash) March 14, 2024 17:11
@jazanne jazanne merged commit 04ccef1 into launchdarkly:main Mar 14, 2024
2 checks passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants