fix(cli): honor task secret opt-in - #461
Conversation
Always serialize the includeSecrets choice so the API default cannot override the CLI's false default. Update the client regression test to cover the explicit false query. Fixes NVIDIA#460 Signed-off-by: goingforstudying-ctrl <goingforstudying@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthrough
ChangesTask secret selection
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to The CLI now explicitly sends the caller’s secret opt-in choice, preventing unintended inclusion of secret names when the flag is omitted. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
TL;DR
Make
nvcf-cli task gethonor its secret-name opt-in by sendingincludeSecrets=falsewhen the flag is not set.Additional Details
The CLI defaults
--include-secretsto false, butClient.GetTaskpreviouslyomitted the query parameter for that value. The NVCT controllers default an
omitted
includeSecretsparameter to true, so a normaltask getrequestcould include secret names without an opt-in.
This change always serializes the caller's boolean choice. The existing client
test now verifies both
includeSecrets=falseandincludeSecrets=trueat theHTTP request boundary.
The API returns secret names, not secret values. The command and flag surface
are unchanged, so no documentation migration or operator action is needed.
There are no dependency, license, or NOTICE changes.
For the Reviewer
Please review the query construction and the two table-driven cases in
internal/client. This PR does not change the server default or the responseshape.
PR #457 changes
cmd/task.go; this PR deliberately stays in the twointernal/clientfiles to avoid overlap.For QA
QA needed: No.
Local validation:
go test ./internal/client -run '^TestGetTaskWithSecrets$' -count=1go test ./internal/client -count=1go test -race ./internal/client -count=1go vet ./...bazel test --remote_cache= //src/clis/nvcf-cli/internal/client:client_test --test_output=errorsbazel test --remote_cache= --nocache_test_results //src/clis/nvcf-cli/... --test_output=errors(20/20 test targets passed)bazel build --remote_cache= //src/clis/nvcf-cli:nvcf-cligit diff --checkIssues
Fixes #460
Checklist
Summary by CodeRabbit