Skip to content

fix(cli): reject negative account quota flags before sending PATCH - #829

Open
rohithb-hub wants to merge 1 commit into
mainfrom
fix/admin-cli-negative-quota-flags
Open

fix(cli): reject negative account quota flags before sending PATCH#829
rohithb-hub wants to merge 1 commit into
mainfrom
fix/admin-cli-negative-quota-flags

Conversation

@rohithb-hub

@rohithb-hub rohithb-hub commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Fixes nvcf-cli admin accounts update silently dropping negative quota values (e.g. --max-functions -2) instead of rejecting them, which previously caused an empty PATCH request and a confusing backend error.

Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)

The four quota flags (--max-functions, --max-tasks, --max-telemetries, --max-registry-credentials) used -1 as an int flag default to mean "not passed," then only added the field to the request when the value was >= 0. A user-supplied negative value was indistinguishable from "not passed" under that check, so it was silently dropped. If it was the only flag given, the CLI sent an empty PATCH and surfaced the backend's generic Either name, credential fields or limits should be specified error instead of identifying the invalid flag.

buildAccountUpdateRequest now takes the *cobra.Command and uses cmd.Flags().Changed(...) to detect whether a flag was actually passed, then explicitly rejects negative values per flag with a message like --max-functions must be greater than or equal to 0 before any request is built. This mirrors the existing --min-instances validation pattern in cmd/deploy.go. Flag defaults changed from -1 to 0 since 0 is now a legitimate "unset" default rather than a sentinel.

For the Reviewer

Main change is in src/clis/nvcf-cli/cmd/admin.go (buildAccountUpdateRequest, runAccountsUpdate). Tests added in src/clis/nvcf-cli/cmd/admin_test.go.

For QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)

  • Added unit tests in admin_test.go covering: no fields provided, valid quotas, 0 as an explicit legitimate value, negative values rejected per flag (all four flags), values exceeding the 50 cap, and a regression test asserting no HTTP request is sent when a negative quota is passed.
  • go build ./... and go test ./cmd/... pass for the changed package.
  • Verified live against a local self-managed k3d cluster: reproduced the original bug on the pre-fix binary (--max-functions=-2 sent an empty PATCH and returned the generic 400), then confirmed the fixed binary rejects all four flags client-side with no request sent, and that a valid update (--max-functions 50) still round-trips correctly end-to-end.

Issues

NO-REF

Checklist

  • I am familiar with the Contributing Guidelines.
  • I have signed off my commits for Developer Certificate of Origin (DCO) compliance.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes. (no user-facing docs changes needed; behavior now matches documented flag semantics)

Summary by CodeRabbit

  • Bug Fixes

    • Account quota updates now correctly support explicit zero values.
    • Negative quota values are rejected with clear, field-specific validation errors.
    • Existing maximum quota limits remain enforced.
    • Invalid quota updates are blocked before any request is sent.
  • Tests

    • Added coverage for valid, zero, negative, maximum, and missing quota values.
    • Added verification for successful account update requests and JSON output.

@rohithb-hub
rohithb-hub requested a review from a team as a code owner August 13, 2026 18:21
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a95f42f1-5f8c-46f2-9ad6-86b4e33b1355

📥 Commits

Reviewing files that changed from the base of the PR and between 6384071 and 14e29df.

📒 Files selected for processing (2)
  • src/clis/nvcf-cli/cmd/admin.go
  • src/clis/nvcf-cli/cmd/admin_test.go

📝 Walkthrough

Walkthrough

Account update quota flags now default to zero and use Cobra flag-presence detection. Negative quotas are rejected with field-specific errors. Tests cover validation, zero values, backend request suppression, PATCH payloads, and JSON output.

Changes

Account quota updates

Layer / File(s) Summary
Quota flag handling and request validation
src/clis/nvcf-cli/cmd/admin.go
Quota flags use zero defaults. Request construction checks whether each flag was supplied, preserves explicit zero values, and rejects negative or excessive quota values.
Quota update test coverage
src/clis/nvcf-cli/cmd/admin_test.go
Tests cover isolated flag setup, request construction, validation errors, prevention of invalid backend requests, valid PATCH requests, and JSON output. Existing flag cleanup uses multiline resets.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to 14e29

Negative quota values are now rejected before an update request is sent, preventing empty PATCH requests and confusing backend errors; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: nvaghela-oss

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and accurately describes the CLI bug fix for rejecting negative account quota flags.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/admin-cli-negative-quota-flags

Comment @coderabbitai help to get the list of available commands.

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.

1 participant