Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA fix was made to the URL construction logic in the diff data URL generation functionality. Previously, query parameters were concatenated directly to the base URL without properly including the query string separator. The change adds a "?" character before the encoded parameters to ensure correct query string formatting. This modification was documented with a corresponding changelog entry. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
infrahub_sdk/branch.py (1)
82-82: Add a regression test for the URL builder.This fix looks correct, but there’s no matching test in this diff. A small unit test around
generate_diff_data_url()would lock in the?separator and cover the optionaltime_from/time_tobranches so this malformed URL shape doesn’t regress again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/branch.py` at line 82, Add a unit test for generate_diff_data_url that asserts the returned URL always contains a single '?' before the query string and correctly includes optional query params when time_from and/or time_to are provided; specifically write tests that call generate_diff_data_url() with (a) only required params to ensure the URL contains "?" then the encoded params, (b) with time_from set, (c) with time_to set, and (d) with both set, asserting the encoded values appear and no extra '?' or malformed separators occur so the current behavior in the function (return url + "?" + urlencode(url_params)) is locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@infrahub_sdk/branch.py`:
- Line 82: Add a unit test for generate_diff_data_url that asserts the returned
URL always contains a single '?' before the query string and correctly includes
optional query params when time_from and/or time_to are provided; specifically
write tests that call generate_diff_data_url() with (a) only required params to
ensure the URL contains "?" then the encoded params, (b) with time_from set, (c)
with time_to set, and (d) with both set, asserting the encoded values appear and
no extra '?' or malformed separators occur so the current behavior in the
function (return url + "?" + urlencode(url_params)) is locked in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dc20e21-9d14-440d-b8ad-74cd4275d045
📒 Files selected for processing (2)
changelog/325.fixed.mdinfrahub_sdk/branch.py
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #865 +/- ##
====================================================
+ Coverage 80.64% 80.66% +0.01%
====================================================
Files 118 118
Lines 10246 10246
Branches 1534 1534
====================================================
+ Hits 8263 8265 +2
+ Misses 1455 1454 -1
+ Partials 528 527 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
Why
The SDK's URL builder was missing a
?separator before query parameters, producing malformed URLs like/api/diff/databranch=test-branch&branch_only=true.Closes #325
What changed
InfraHubBranchManagerBase.generate_diff_data_urlwas concatenating query params without a?separator; now correctly produces/api/diff/data?branch=...&branch_only=....How to review
python_sdk/infrahub_sdk/branch.py:82— the one-character URL fix (+ "?" +)Impact & rollout
Checklist
uv run towncrier create ...)Summary by CodeRabbit