fix: CR api responses matching Limes validations#843
Merged
Conversation
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
📝 WalkthroughWalkthroughThis PR standardizes resource topology selection and refactors quota configuration in the commitments service. All resources now unconditionally use ChangesResource topology and quota standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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)
internal/scheduling/reservations/commitments/usage.go (1)
514-526: ⚡ Quick winMisleading function name and comment.
The helper
lookupQuotaGiBand its comment "stored in GiB" are misleading because the function is used for cores (line 564) and instances (line 589), which are stored as counts, not GiB. Only RAM quotas are actually in GiB and require unit conversion viafg.GiBToDeclaredUnits(...).Consider renaming to
lookupQuotaand updating the comment to reflect that each resource stores quota in its own unit (GiB for RAM, count for cores/instances).♻️ Proposed fix
- // helper: look up stored quota for a resource in a given AZ (stored in GiB). + // helper: look up stored quota for a resource in a given AZ. + // RAM quotas are stored in GiB; cores and instances are stored as counts. // Returns -1 (infinite) if not found. Unit conversion is done by the caller. - lookupQuotaGiB := func(resourceName string, az liquid.AvailabilityZone) int64 { + lookupQuota := func(resourceName string, az liquid.AvailabilityZone) int64 { if quotaByResourceAZ == nil { return -1 }Then update the call sites:
- report.Quota = Some(fg.GiBToDeclaredUnits(lookupQuotaGiB(string(ramResourceName), az))) + report.Quota = Some(fg.GiBToDeclaredUnits(lookupQuota(string(ramResourceName), az)))- report.Quota = Some(lookupQuotaGiB(string(coresResourceName), az)) + report.Quota = Some(lookupQuota(string(coresResourceName), az))- report.Quota = Some(lookupQuotaGiB(string(instancesResourceName), az)) + report.Quota = Some(lookupQuota(string(instancesResourceName), az))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/reservations/commitments/usage.go` around lines 514 - 526, The helper lookupQuotaGiB is misleading because only RAM quotas are in GiB; rename the function to lookupQuota and update its comment to say it returns stored quota in the resource's native units (GiB for RAM, counts for cores/instances) and that unit conversion is the caller's responsibility; update all call sites that used lookupQuotaGiB (e.g., the RAM path that still uses fg.GiBToDeclaredUnits(...), and the cores/instances paths) to call lookupQuota and ensure RAM conversion remains applied while cores/instances treat the returned value as a count, and update any identifier references to the old name accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/scheduling/reservations/commitments/usage.go`:
- Around line 514-526: The helper lookupQuotaGiB is misleading because only RAM
quotas are in GiB; rename the function to lookupQuota and update its comment to
say it returns stored quota in the resource's native units (GiB for RAM, counts
for cores/instances) and that unit conversion is the caller's responsibility;
update all call sites that used lookupQuotaGiB (e.g., the RAM path that still
uses fg.GiBToDeclaredUnits(...), and the cores/instances paths) to call
lookupQuota and ensure RAM conversion remains applied while cores/instances
treat the returned value as a count, and update any identifier references to the
old name accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bb116b3-6847-42bb-96a8-b11b3040ed11
📒 Files selected for processing (3)
internal/scheduling/reservations/commitments/api/info.gointernal/scheduling/reservations/commitments/api/info_test.gointernal/scheduling/reservations/commitments/usage.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.2% |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.