fix: allow user to only set one field for ng or cz#211
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR removes the constraint that ChangesRemove reserved pair validation for enrollment flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5819fff0b
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleetint/enroll_test.go (1)
343-418: ⚡ Quick winConsider adding a test case for both flags cleared.
The test comprehensively covers independent updates, but could include a case where both flags are explicitly set to empty strings to demonstrate that clearing both fields independently is supported.
📝 Suggested additional test case
{ name: "clear node-group and omit compute-zone", nodeGroupArg: strPtr(""), computeZoneArg: nil, expectNodeGroup: strPtr(""), expectComputeZone: nil, }, + { + name: "clear both node-group and compute-zone", + nodeGroupArg: strPtr(""), + computeZoneArg: strPtr(""), + expectNodeGroup: strPtr(""), + expectComputeZone: strPtr(""), + }, }🤖 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_test.go` around lines 343 - 418, Add a test variant to TestValidatedOptionalMetadataFlagValueSupportsIndependentReservedUpdates that covers the scenario where both "node-group" and "compute-zone" flags are explicitly set to empty strings; in the test setup for this case call flagSet.Set("node-group", "") and flagSet.Set("compute-zone", ""), then assert that validatedOptionalMetadataFlagValue(cliContext, "node-group", "Node group") and validatedOptionalMetadataFlagValue(cliContext, "compute-zone", "Compute zone") both return pointers to empty strings (expectNodeGroup and expectComputeZone should be strPtr("")). Ensure the new case is added to the tests slice alongside the other cases so it runs with the existing t.Run loop.
🤖 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_test.go`:
- Around line 343-418: Add a test variant to
TestValidatedOptionalMetadataFlagValueSupportsIndependentReservedUpdates that
covers the scenario where both "node-group" and "compute-zone" flags are
explicitly set to empty strings; in the test setup for this case call
flagSet.Set("node-group", "") and flagSet.Set("compute-zone", ""), then assert
that validatedOptionalMetadataFlagValue(cliContext, "node-group", "Node group")
and validatedOptionalMetadataFlagValue(cliContext, "compute-zone", "Compute
zone") both return pointers to empty strings (expectNodeGroup and
expectComputeZone should be strPtr("")). Ensure the new case is added to the
tests slice alongside the other cases so it runs with the existing t.Run loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 37c335b8-350b-41b7-b933-acb523e05331
📒 Files selected for processing (3)
cmd/fleetint/enroll.gocmd/fleetint/enroll_test.godeployments/helm/fleet-intelligence-agent/values.yaml
💤 Files with no reviewable changes (1)
- cmd/fleetint/enroll.go
jingxiang-z
left a comment
jingxiang-z
left a comment
There was a problem hiding this comment.
@ambermingxin You can add your metadata key underscore change in the PR also.
jingxiang-z
changed the title
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/agentstate/sqlite_test.go (1)
93-93: ⚡ Quick winPrefer the package constant for consistency.
Using the hardcoded string
"node_group"instead of the existingMetadataKeyNodeGroupconstant creates a maintenance risk. If the constant value changes, this test could pass while verifying the wrong key.♻️ Refactor to use the constant
- value, err = pkgmetadata.ReadMetadata(ctx, db, "node_group") + value, err = pkgmetadata.ReadMetadata(ctx, db, MetadataKeyNodeGroup)🤖 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/agentstate/sqlite_test.go` at line 93, The test calls pkgmetadata.ReadMetadata with a hardcoded key string "node_group" which risks divergence from the canonical constant; update the call to use the package constant MetadataKeyNodeGroup instead so the test always targets the same key as application code (i.e., replace the literal "node_group" passed to pkgmetadata.ReadMetadata with MetadataKeyNodeGroup).
🤖 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/agentstate/sqlite_test.go`:
- Line 93: The test calls pkgmetadata.ReadMetadata with a hardcoded key string
"node_group" which risks divergence from the canonical constant; update the call
to use the package constant MetadataKeyNodeGroup instead so the test always
targets the same key as application code (i.e., replace the literal "node_group"
passed to pkgmetadata.ReadMetadata with MetadataKeyNodeGroup).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 13b404bc-bd14-40f1-9b77-5ac20b0397ba
📒 Files selected for processing (5)
internal/agentstate/sqlite_test.gointernal/agentstate/state.gointernal/enrollment/enrollment.gointernal/exporter/exporter.gointernal/inventory/sink/backend.go
✅ Files skipped from review due to trivial changes (4)
- internal/agentstate/state.go
- internal/inventory/sink/backend.go
- internal/exporter/exporter.go
- internal/enrollment/enrollment.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
|
Actionable comments posted: 0 |
2 participants
Description
Checklist
Summary by CodeRabbit
New Features
Bug Fixes