feat: add validation for ng/cz input#206
Conversation
Signed-off-by: Amber Xue <ambermingxin@nvidia.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 (1)
📝 WalkthroughWalkthroughThis PR tightens enrollment metadata validation and simplifies metadata persistence. The enrollment command now validates ChangesEnrollment metadata validation and storage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcc526e5c9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/inventory/sink/backend.go`:
- Line 104: The current log line uses log.Logger.Infow to emit the entire
NodeUpsertRequest (variable req), which may contain sensitive
host/system/network identifiers; change the logging to only include a minimal,
non-sensitive subset (e.g., nodeUUID and a high-level action/status) or a
redacted/sanitized version of req, or demote the full payload to Debug level.
Locate the Infow call that logs "inventory export to backend" and replace it so
it does not print req raw—either construct a small map of safe fields to log
(avoid host/system/network ids) or call log.Logger.Debugw with a scrubbed copy
of req instead. Ensure the unique symbols referenced are log.Logger.Infow, the
message "inventory export to backend", the req variable (NodeUpsertRequest) and
nodeUUID so reviewers can find and update the log correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f3c80ecc-5b65-4e94-8be2-e7f114fc4dd7
📒 Files selected for processing (5)
cmd/fleetint/enroll.gocmd/fleetint/enroll_test.gointernal/inventory/sink/backend.gothird_party/fleet-intelligence-sdk/pkg/metadata/metadata.gothird_party/fleet-intelligence-sdk/pkg/metadata/metadata_test.go
Signed-off-by: Amber Xue <ambermingxin@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleetint/enroll.go (1)
163-178: ⚡ Quick winAdd a comment explaining the paired validation business rule.
The function correctly enforces that
--node-groupand--compute-zonemust be both omitted, both empty, or both non-empty, but there's no comment explaining why this pairing constraint exists. Adding documentation will help future maintainers understand the business logic behind this validation.📝 Suggested documentation
+// validateReservedPairMetadata enforces that node-group and compute-zone are used +// together as a composite assignment: both must be omitted (no assignment), both +// empty (clear assignment), or both non-empty (set assignment). func validateReservedPairMetadata(nodeGroup, computeZone *string) error {🤖 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 `@cmd/fleetint/enroll.go` around lines 163 - 178, Add a concise comment above the validateReservedPairMetadata function that documents the business rule: explain that --node-group and --compute-zone form a paired option set and therefore must be specified together (both omitted, both empty strings, or both non-empty) because the system treats them as a linked reservation identifier (nodeGroup + computeZone) and partial values are ambiguous/invalid; reference the parameter names nodeGroup and computeZone and the function validateReservedPairMetadata so future maintainers understand the intended constraint and rationale.
🤖 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 `@cmd/fleetint/enroll.go`:
- Around line 163-178: Add a concise comment above the
validateReservedPairMetadata function that documents the business rule: explain
that --node-group and --compute-zone form a paired option set and therefore must
be specified together (both omitted, both empty strings, or both non-empty)
because the system treats them as a linked reservation identifier (nodeGroup +
computeZone) and partial values are ambiguous/invalid; reference the parameter
names nodeGroup and computeZone and the function validateReservedPairMetadata so
future maintainers understand the intended constraint and rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 27730244-7959-4cbb-9a54-bb63c646ddf3
📒 Files selected for processing (2)
cmd/fleetint/enroll.gocmd/fleetint/enroll_test.go
Signed-off-by: Amber Xue <ambermingxin@nvidia.com>
Description
Checklist
Summary by CodeRabbit
New Features
Bug Fixes