ECOPROJECT-4282 | feat: add cluster drs information into the api#1171
Merged
Conversation
itroyano
requested review from
AvielSegev and
nirarg
and removed request for
a team
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds cluster-level DRS fields across API schema and generated types, extends DB schema and RVTools ingest to populate DRS columns, implements query and parser to fetch cluster features, wires inventory builder and API conversion to include ClusterFeatures, and adds tests for parsing and defaults. ChangesCluster DRS Features Support
Sequence Diagram(s)sequenceDiagram
participant InventoryBuilder
participant Parser
participant QueryBuilder
participant DuckDB
participant Converter
InventoryBuilder->>Parser: ClusterFeatures(ctx, clusterName)
Parser->>QueryBuilder: ClusterFeaturesQuery(clusterName)
QueryBuilder-->>Parser: SQL string
Parser->>DuckDB: QueryRowContext(SQL)
DuckDB-->>Parser: row (DrsEnabled, drsMode, StorageDrsEnabled) / ErrNoRows
Parser-->>InventoryBuilder: *models.ClusterFeatures or error
InventoryBuilder->>Converter: toAPIInventoryData(inventory with ClusterFeatures)
Converter-->>InventoryBuilder: api.InventoryData with ClusterFeatures
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 unit tests (beta)
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.
Actionable comments posted: 6
🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 1271-1351: The test TestBuildInventory_ClusterFeatures currently
asserts only ClusterFeatures.DrsEnabled; extend it to also exercise and assert
ClusterFeatures.DrsMode and ClusterFeatures.StorageDrsEnabled by adding
appropriate columns to the vClustersRows test data (e.g., "drsMode": with values
like "fullyAutomated", "partiallyAutomated", "manual", ""/unknown to map to
None) and "storageDrs": with true/false/empty values, then after
parser.IngestRvTools and parser.BuildInventory locate each cluster (as done now)
and add require/assert statements that 1) ClusterFeatures.DrsMode is non-nil and
equals the expected API enum (FullyAutomated/PartiallyAutomated/Manual/None) and
2) ClusterFeatures.StorageDrsEnabled is non-nil and matches the expected boolean
parsing, using the same patterns as the existing DrsEnabled assertions.
- Line 96: Update the vClusterHeaders test constant to include the missing
columns expected by the ingestion template: change vClusterHeaders to
[]string{"Name", "Object ID", "drs", "DrsDefaultVmBehavior", "Storage DRS"} and
add corresponding test values in the vCluster test rows so the
inventory_builder_test.go test data matches the ingest_rvtools.go.tmpl schema
(ensure any uses of vClusterHeaders or test rows in inventory_builder_test.go
are updated to include the new columns).
In `@pkg/duckdb_parser/templates/cluster_features_query.go.tmpl`:
- Around line 7-9: The WHERE clause in the ClusterFeaturesQuery template uses
raw ClusterName interpolation causing SQL injection; fix by applying the
existing escapeSQLString function to the clusterName before rendering the
template (i.e., in the code path that constructs data for ClusterFeaturesQuery
in pkg/duckdb_parser/builder.go replace the raw clusterName with
escapeSQLString(clusterName) so the template's "ClusterName" field is the
escaped value), ensuring you don't double-escape if other callers already escape
it.
In `@pkg/duckdb_parser/templates/create_schema.go.tmpl`:
- Around line 156-158: The template currently sets the "DrsDefaultVmBehavior"
column default to 'manual', which conflicts with the API contract that defaults
drsMode to "None"; update the CREATE TABLE template in
pkg/duckdb_parser/templates/create_schema.go.tmpl so the "DrsDefaultVmBehavior"
VARCHAR DEFAULT value is "None" (matching the API) and ensure any related SQL
generation or seed logic that assumes 'manual' is adjusted to use "None" as the
missing/unknown default.
In `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl`:
- Around line 282-293: The vCluster ingestion assumes the headers "drs" and
"Storage DRS" always exist and will error if missing; fix by reading the sheet
into a raw source and defensively COALESCE’ing missing columns before INSERT:
create/alias the read_xlsx result (the existing raw-table pattern) and in the
INSERT INTO vcluster SELECT use COALESCE(raw."drs", '') and
COALESCE(raw."Storage DRS", '') when evaluating the boolean CASE expressions and
COALESCE(NULLIF(TRIM(raw."DrsDefaultVmBehavior"), ''), 'manual') for the default
behavior, so missing headers default safely instead of causing a failure.
In `@pkg/inventory/converters/to_api.go`:
- Around line 40-56: The DrsMode mapping in the converter currently does
case-sensitive comparisons on d.ClusterFeatures.DrsMode and falls back to
api.None for mixed-case inputs; normalize the input (trim and convert to
lower-case) before the switch on *d.ClusterFeatures.DrsMode and then match
against lower-case variants like "fully automated", "partially automated",
"manual" to set clusterFeatures.DrsMode to pointers of api.FullyAutomated,
api.PartiallyAutomated, api.Manual (or api.None for unknowns); update the switch
in the same block that references d.ClusterFeatures.DrsMode and
clusterFeatures.DrsMode so all case variants are handled consistently.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 4fb03fe8-097d-4bae-825e-c29eebdae5f8
📒 Files selected for processing (14)
api/v1alpha1/agent/spec.gen.goapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gopkg/duckdb_parser/builder.gopkg/duckdb_parser/inventory_builder.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/models/inventory.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/templates/cluster_features_query.go.tmplpkg/duckdb_parser/templates/create_schema.go.tmplpkg/duckdb_parser/templates/ingest_rvtools.go.tmplpkg/inventory/converters/to_api.gopkg/inventory/model.go
Comment on lines
+1271
to
+1351
| // TestBuildInventory_ClusterFeatures tests that cluster features including DrsEnabled | ||
| // are properly parsed from vCluster sheet and populated in the inventory. | ||
| func TestBuildInventory_ClusterFeatures(t *testing.T) { | ||
| parser, _, cleanup := setupTestParser(t, &testValidator{}) | ||
| defer cleanup() | ||
|
|
||
| vms := []map[string]string{ | ||
| {"VM": "vm-1", "VM ID": "vm-001", "VI SDK UUID": "uuid-1", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster-drs-enabled", "Datacenter": "dc1"}, | ||
| {"VM": "vm-2", "VM ID": "vm-002", "VI SDK UUID": "uuid-2", "Host": "esxi-host-2", "CPUs": "2", "Memory": "4096", "Powerstate": "poweredOn", "Cluster": "cluster-drs-disabled", "Datacenter": "dc1"}, | ||
| {"VM": "vm-3", "VM ID": "vm-003", "VI SDK UUID": "uuid-3", "Host": "esxi-host-3", "CPUs": "8", "Memory": "16384", "Powerstate": "poweredOn", "Cluster": "cluster-no-drs-data", "Datacenter": "dc1"}, | ||
| } | ||
| hosts := []map[string]string{ | ||
| {"Datacenter": "dc1", "Cluster": "cluster-drs-enabled", "# Cores": "8", "# CPU": "2", "Object ID": "host-001", "# Memory": "32768", "Model": "ESXi", "Vendor": "VMware", "Host": "esxi-host-1", "Config status": "green"}, | ||
| {"Datacenter": "dc1", "Cluster": "cluster-drs-disabled", "# Cores": "16", "# CPU": "2", "Object ID": "host-002", "# Memory": "65536", "Model": "ESXi", "Vendor": "VMware", "Host": "esxi-host-2", "Config status": "green"}, | ||
| {"Datacenter": "dc1", "Cluster": "cluster-no-drs-data", "# Cores": "12", "# CPU": "2", "Object ID": "host-003", "# Memory": "49152", "Model": "ESXi", "Vendor": "VMware", "Host": "esxi-host-3", "Config status": "green"}, | ||
| } | ||
|
|
||
| // Custom vCluster data with specific DRS settings | ||
| vClustersRows := []map[string]string{ | ||
| {"Name": "cluster-drs-enabled", "Object ID": "domain-c1", "drs": "true"}, | ||
| {"Name": "cluster-drs-disabled", "Object ID": "domain-c2", "drs": "false"}, | ||
| {"Name": "cluster-no-drs-data", "Object ID": "domain-c3", "drs": ""}, // Empty DRS value should default to false | ||
| } | ||
|
|
||
| vDatastoreRows := []map[string]string{ | ||
| {"Hosts": "esxi-host-1", "Address": "10.0.0.1", "Name": "datastore1", "Object ID": "datastore-001", "Free MiB": "524288", "MHA": "false", "Capacity MiB": "1048576", "Type": "VMFS"}, | ||
| } | ||
|
|
||
| sheets := []ExcelSheet{ | ||
| NewExcelSheet("vInfo", vInfoHeaders, vms), | ||
| NewExcelSheet("vHost", vHostHeaders, hosts), | ||
| NewExcelSheet("vDatastore", vDatastoreHeaders, vDatastoreRows), | ||
| NewExcelSheet("vCluster", vClusterHeaders, vClustersRows), | ||
| } | ||
| tmpFile := createTestExcel(t, sheets...) | ||
|
|
||
| ctx := context.Background() | ||
| _, err := parser.IngestRvTools(ctx, tmpFile) | ||
| require.NoError(t, err) | ||
|
|
||
| inv, err := parser.BuildInventory(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| // Should have 3 clusters | ||
| assert.Len(t, inv.Clusters, 3) | ||
|
|
||
| // Find each cluster and verify their DRS settings | ||
| var drsEnabledCluster, drsDisabledCluster, noDrsDataCluster *inventory.InventoryData | ||
| for clusterID, cluster := range inv.Clusters { | ||
| if cluster.ClusterFeatures != nil { | ||
| switch cluster.ClusterFeatures.Name { | ||
| case "cluster-drs-enabled": | ||
| drsEnabledCluster = &cluster | ||
| case "cluster-drs-disabled": | ||
| drsDisabledCluster = &cluster | ||
| case "cluster-no-drs-data": | ||
| noDrsDataCluster = &cluster | ||
| } | ||
| } else { | ||
| t.Logf("Cluster %s has no cluster features", clusterID) | ||
| } | ||
| } | ||
|
|
||
| // Verify cluster with DRS enabled | ||
| require.NotNil(t, drsEnabledCluster, "Should find cluster-drs-enabled") | ||
| require.NotNil(t, drsEnabledCluster.ClusterFeatures, "cluster-drs-enabled should have cluster features") | ||
| require.NotNil(t, drsEnabledCluster.ClusterFeatures.DrsEnabled, "cluster-drs-enabled should have DrsEnabled set") | ||
| assert.True(t, *drsEnabledCluster.ClusterFeatures.DrsEnabled, "cluster-drs-enabled should have DRS enabled") | ||
|
|
||
| // Verify cluster with DRS disabled | ||
| require.NotNil(t, drsDisabledCluster, "Should find cluster-drs-disabled") | ||
| require.NotNil(t, drsDisabledCluster.ClusterFeatures, "cluster-drs-disabled should have cluster features") | ||
| require.NotNil(t, drsDisabledCluster.ClusterFeatures.DrsEnabled, "cluster-drs-disabled should have DrsEnabled set") | ||
| assert.False(t, *drsDisabledCluster.ClusterFeatures.DrsEnabled, "cluster-drs-disabled should have DRS disabled") | ||
|
|
||
| // Verify cluster with missing DRS data (should default to false) | ||
| require.NotNil(t, noDrsDataCluster, "Should find cluster-no-drs-data") | ||
| require.NotNil(t, noDrsDataCluster.ClusterFeatures, "cluster-no-drs-data should have cluster features") | ||
| require.NotNil(t, noDrsDataCluster.ClusterFeatures.DrsEnabled, "cluster-no-drs-data should have DrsEnabled set") | ||
| assert.False(t, *noDrsDataCluster.ClusterFeatures.DrsEnabled, "cluster-no-drs-data should default to DRS disabled") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding test coverage for DrsMode and StorageDrsEnabled fields.
The test thoroughly validates DrsEnabled boolean parsing, but ClusterFeatures also contains DrsMode and StorageDrsEnabled fields (per the converter in to_api.go). These fields are not tested here, leaving their parsing and conversion logic unverified.
Consider extending the test to verify:
- DrsMode string values are correctly parsed from the vCluster sheet
- DrsMode is correctly converted to API enum values (FullyAutomated, PartiallyAutomated, Manual, None)
- StorageDrsEnabled boolean is parsed similarly to DrsEnabled
🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go` around lines 1271 - 1351, The
test TestBuildInventory_ClusterFeatures currently asserts only
ClusterFeatures.DrsEnabled; extend it to also exercise and assert
ClusterFeatures.DrsMode and ClusterFeatures.StorageDrsEnabled by adding
appropriate columns to the vClustersRows test data (e.g., "drsMode": with values
like "fullyAutomated", "partiallyAutomated", "manual", ""/unknown to map to
None) and "storageDrs": with true/false/empty values, then after
parser.IngestRvTools and parser.BuildInventory locate each cluster (as done now)
and add require/assert statements that 1) ClusterFeatures.DrsMode is non-nil and
equals the expected API enum (FullyAutomated/PartiallyAutomated/Manual/None) and
2) ClusterFeatures.StorageDrsEnabled is non-nil and matches the expected boolean
parsing, using the same patterns as the existing DrsEnabled assertions.
9bfaad5 to
2dc0da5
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl (1)
282-301:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
vcluster_rawafter the insert to avoid re-ingest failures.
vcluster_rawis created but never dropped. A subsequent ingest on the same DB/session can fail on table creation, and the temp table is unnecessary after theINSERT.🔧 Proposed fix
FROM vcluster_raw; +DROP TABLE vcluster_raw;🤖 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 `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl` around lines 282 - 301, The temporary table vcluster_raw is created for staging but never dropped, causing subsequent ingests to fail on CREATE TABLE; after the INSERT INTO vcluster completes, add a DROP TABLE IF EXISTS vcluster_raw (or equivalent) to remove the temp table; locate the CREATE TABLE vcluster_raw and the INSERT INTO vcluster statements in the template and append a DROP TABLE IF EXISTS vcluster_raw command immediately after the INSERT to ensure idempotent re-ingests.
♻️ Duplicate comments (2)
pkg/duckdb_parser/inventory_builder_test.go (2)
1479-1559: 🧹 Nitpick | 🔵 TrivialConsider adding test coverage for DrsMode and StorageDrsEnabled fields.
The test validates
DrsEnabledboolean parsing, butClusterFeaturesalso containsDrsModeandStorageDrsEnabledfields. These fields are not tested here, leaving their parsing and conversion logic unverified.🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go` around lines 1479 - 1559, The test TestBuildInventory_ClusterFeatures only asserts DrsEnabled but misses coverage for ClusterFeatures.DrsMode and ClusterFeatures.StorageDrsEnabled; update the vClustersRows in the test to include columns for "drsMode" (or whatever header your parser expects) and "storageDrs" (or the parser header for StorageDrsEnabled) with representative values for the three clusters, then after BuildInventory add assertions that drsEnabledCluster.ClusterFeatures.DrsMode and .StorageDrsEnabled (and similarly for drsDisabledCluster and noDrsDataCluster) are non-nil and have the expected values (e.g., specific mode strings and true/false for storage DRS) to validate parsing logic in the parser.IngestRvTools/BuildInventory path.
96-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing vCluster columns to match ingestion template schema.
The
vClusterHeadersis missing"DrsDefaultVmBehavior"and"Storage DRS"columns that the ingestion template expects to read from the vCluster sheet.🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go` at line 96, The vClusterHeaders slice used in tests (vClusterHeaders in inventory_builder_test.go) is missing the "DrsDefaultVmBehavior" and "Storage DRS" columns required by the ingestion template; update the vClusterHeaders declaration to include those two headers (in the same order the ingestion template expects) so the test schema matches the real vCluster sheet.
🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 1479-1483: Add t.Parallel() at the start of the
TestBuildInventory_ClusterFeatures test to enable parallel execution; update the
TestBuildInventory_ClusterFeatures function (before calling setupTestParser) to
call t.Parallel() so the test runs safely in parallel with others while keeping
the existing setupTestParser and deferred cleanup intact.
- Around line 1563-1589: The subtests in
TestBuildInventory_ClusterFeaturesFlexibleParsing are independent and safe to
run concurrently; inside the t.Run callback (the anonymous func passed to t.Run)
add a call to t.Parallel() as the first statement to enable parallel execution
of each case, ensuring you still perform per-subtest setup/teardown (the
in-memory DB creation and cleanup) inside that callback so resource isolation is
preserved.
In `@pkg/duckdb_parser/inventory_builder.go`:
- Around line 93-99: The code currently returns an error when
p.buildClusterData(ctx, filters.Cluster) fails, causing the whole cluster
inventory to be dropped; change this to treat cluster-features enrichment as
non-fatal: call p.buildClusterData, and if it returns an error, log a warning
(using the repository's logger or fmt) that includes the error and continue
without setting cluster (or leave cluster as-is/empty) instead of returning
fmt.Errorf; ensure the call site around filters.Cluster uses the non-fatal
behavior so the inventory generation proceeds even if p.buildClusterData fails.
In `@pkg/inventory/model.go`:
- Around line 3-5: InventoryData currently imports and uses the parser-layer
type models.Cluster, coupling pkg/inventory to pkg/duckdb_parser/models; create
a local inventory cluster/features struct (e.g., InventoryCluster or
ClusterFeatures) inside pkg/inventory/model.go that contains only the fields
needed by InventoryData, replace usages of models.Cluster with this new local
type in InventoryData and related functions, and update the parser/converter
code (where models.Cluster is produced) to map/convert models.Cluster ->
inventory.ClusterFeatures at the boundary so pkg/inventory no longer imports or
depends on duckdb_parser models.Cluster.
---
Outside diff comments:
In `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl`:
- Around line 282-301: The temporary table vcluster_raw is created for staging
but never dropped, causing subsequent ingests to fail on CREATE TABLE; after the
INSERT INTO vcluster completes, add a DROP TABLE IF EXISTS vcluster_raw (or
equivalent) to remove the temp table; locate the CREATE TABLE vcluster_raw and
the INSERT INTO vcluster statements in the template and append a DROP TABLE IF
EXISTS vcluster_raw command immediately after the INSERT to ensure idempotent
re-ingests.
---
Duplicate comments:
In `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 1479-1559: The test TestBuildInventory_ClusterFeatures only
asserts DrsEnabled but misses coverage for ClusterFeatures.DrsMode and
ClusterFeatures.StorageDrsEnabled; update the vClustersRows in the test to
include columns for "drsMode" (or whatever header your parser expects) and
"storageDrs" (or the parser header for StorageDrsEnabled) with representative
values for the three clusters, then after BuildInventory add assertions that
drsEnabledCluster.ClusterFeatures.DrsMode and .StorageDrsEnabled (and similarly
for drsDisabledCluster and noDrsDataCluster) are non-nil and have the expected
values (e.g., specific mode strings and true/false for storage DRS) to validate
parsing logic in the parser.IngestRvTools/BuildInventory path.
- Line 96: The vClusterHeaders slice used in tests (vClusterHeaders in
inventory_builder_test.go) is missing the "DrsDefaultVmBehavior" and "Storage
DRS" columns required by the ingestion template; update the vClusterHeaders
declaration to include those two headers (in the same order the ingestion
template expects) so the test schema matches the real vCluster sheet.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ebf3f555-c5fe-4f78-a5a0-09ba4db3971a
📒 Files selected for processing (14)
api/v1alpha1/agent/spec.gen.goapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gopkg/duckdb_parser/builder.gopkg/duckdb_parser/inventory_builder.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/models/inventory.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/templates/cluster_features_query.go.tmplpkg/duckdb_parser/templates/create_schema.go.tmplpkg/duckdb_parser/templates/ingest_rvtools.go.tmplpkg/inventory/converters/to_api.gopkg/inventory/model.go
Comment on lines
+1479
to
+1483
| // TestBuildInventory_ClusterFeatures tests that cluster features including DrsEnabled | ||
| // are properly parsed from vCluster sheet and populated in the inventory. | ||
| func TestBuildInventory_ClusterFeatures(t *testing.T) { | ||
| parser, _, cleanup := setupTestParser(t, &testValidator{}) | ||
| defer cleanup() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider enabling parallel test execution.
This test is fully isolated with its own in-memory database and proper cleanup, making it safe to run in parallel with other tests.
⚡ Enable parallelism
func TestBuildInventory_ClusterFeatures(t *testing.T) {
+ t.Parallel()
parser, _, cleanup := setupTestParser(t, &testValidator{})
defer cleanup()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TestBuildInventory_ClusterFeatures tests that cluster features including DrsEnabled | |
| // are properly parsed from vCluster sheet and populated in the inventory. | |
| func TestBuildInventory_ClusterFeatures(t *testing.T) { | |
| parser, _, cleanup := setupTestParser(t, &testValidator{}) | |
| defer cleanup() | |
| // TestBuildInventory_ClusterFeatures tests that cluster features including DrsEnabled | |
| // are properly parsed from vCluster sheet and populated in the inventory. | |
| func TestBuildInventory_ClusterFeatures(t *testing.T) { | |
| t.Parallel() | |
| parser, _, cleanup := setupTestParser(t, &testValidator{}) | |
| defer cleanup() |
🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go` around lines 1479 - 1483, Add
t.Parallel() at the start of the TestBuildInventory_ClusterFeatures test to
enable parallel execution; update the TestBuildInventory_ClusterFeatures
function (before calling setupTestParser) to call t.Parallel() so the test runs
safely in parallel with others while keeping the existing setupTestParser and
deferred cleanup intact.
Comment on lines
+1563
to
+1589
| func TestBuildInventory_ClusterFeaturesFlexibleParsing(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| drsValue string | ||
| expectedEnabled bool | ||
| }{ | ||
| {"true lowercase", "true", true}, | ||
| {"TRUE uppercase", "TRUE", true}, | ||
| {"True mixed case", "True", true}, | ||
| {"1 numeric", "1", true}, | ||
| {"yes lowercase", "yes", true}, | ||
| {"YES uppercase", "YES", true}, | ||
| {"enabled lowercase", "enabled", true}, | ||
| {"ENABLED uppercase", "ENABLED", true}, | ||
| {"false lowercase", "false", false}, | ||
| {"FALSE uppercase", "FALSE", false}, | ||
| {"0 numeric", "0", false}, | ||
| {"no lowercase", "no", false}, | ||
| {"disabled", "disabled", false}, | ||
| {"empty string", "", false}, | ||
| {"whitespace only", " ", false}, | ||
| {"random text", "random", false}, | ||
| {"null-like", "null", false}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider enabling parallel test execution.
This test is fully isolated with each subtest creating its own in-memory database and proper cleanup, making it safe to run in parallel.
⚡ Enable parallelism
func TestBuildInventory_ClusterFeaturesFlexibleParsing(t *testing.T) {
+ t.Parallel()
+
testCases := []struct {
name string
drsValue string
expectedEnabled bool
}{Consider also adding t.Parallel() inside the t.Run callback at line 1589 if subtests are independent.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestBuildInventory_ClusterFeaturesFlexibleParsing(t *testing.T) { | |
| testCases := []struct { | |
| name string | |
| drsValue string | |
| expectedEnabled bool | |
| }{ | |
| {"true lowercase", "true", true}, | |
| {"TRUE uppercase", "TRUE", true}, | |
| {"True mixed case", "True", true}, | |
| {"1 numeric", "1", true}, | |
| {"yes lowercase", "yes", true}, | |
| {"YES uppercase", "YES", true}, | |
| {"enabled lowercase", "enabled", true}, | |
| {"ENABLED uppercase", "ENABLED", true}, | |
| {"false lowercase", "false", false}, | |
| {"FALSE uppercase", "FALSE", false}, | |
| {"0 numeric", "0", false}, | |
| {"no lowercase", "no", false}, | |
| {"disabled", "disabled", false}, | |
| {"empty string", "", false}, | |
| {"whitespace only", " ", false}, | |
| {"random text", "random", false}, | |
| {"null-like", "null", false}, | |
| } | |
| for _, tc := range testCases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| func TestBuildInventory_ClusterFeaturesFlexibleParsing(t *testing.T) { | |
| t.Parallel() | |
| testCases := []struct { | |
| name string | |
| drsValue string | |
| expectedEnabled bool | |
| }{ | |
| {"true lowercase", "true", true}, | |
| {"TRUE uppercase", "TRUE", true}, | |
| {"True mixed case", "True", true}, | |
| {"1 numeric", "1", true}, | |
| {"yes lowercase", "yes", true}, | |
| {"YES uppercase", "YES", true}, | |
| {"enabled lowercase", "enabled", true}, | |
| {"ENABLED uppercase", "ENABLED", true}, | |
| {"false lowercase", "false", false}, | |
| {"FALSE uppercase", "FALSE", false}, | |
| {"0 numeric", "0", false}, | |
| {"no lowercase", "no", false}, | |
| {"disabled", "disabled", false}, | |
| {"empty string", "", false}, | |
| {"whitespace only", " ", false}, | |
| {"random text", "random", false}, | |
| {"null-like", "null", false}, | |
| } | |
| for _, tc := range testCases { | |
| t.Run(tc.name, func(t *testing.T) { |
🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go` around lines 1563 - 1589, The
subtests in TestBuildInventory_ClusterFeaturesFlexibleParsing are independent
and safe to run concurrently; inside the t.Run callback (the anonymous func
passed to t.Run) add a call to t.Parallel() as the first statement to enable
parallel execution of each case, ensuring you still perform per-subtest
setup/teardown (the in-memory DB creation and cleanup) inside that callback so
resource isolation is preserved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl (1)
282-301:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
vcluster_rawafter insert to avoid stale staging-table failures.
vcluster_rawis created withoutIF NOT EXISTSand never dropped. A subsequent ingest in the same DB/session can fail on table creation and keep unnecessary staging data around.🔧 Proposed fix
INSERT INTO vcluster ("Name", "Object ID", "DrsEnabled", "DrsDefaultVmBehavior", "StorageDrsEnabled") SELECT "Name", "Object ID", CASE WHEN LOWER(TRIM(COALESCE("drs", ''))) IN ('true', '1', 'yes', 'enabled') THEN true ELSE false END as "DrsEnabled", COALESCE(NULLIF(TRIM("DrsDefaultVmBehavior"), ''), 'None') as "DrsDefaultVmBehavior", CASE WHEN LOWER(TRIM(COALESCE("Storage DRS", ''))) IN ('true', '1', 'yes', 'enabled') THEN true ELSE false END as "StorageDrsEnabled" FROM vcluster_raw; + +DROP TABLE vcluster_raw;🤖 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 `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl` around lines 282 - 301, The staging table vcluster_raw is created but never removed, causing subsequent ingests to fail or leave stale data; update the template around the vcluster_raw lifecycle (the CREATE TABLE vcluster_raw ... and subsequent INSERT INTO vcluster ...) to ensure the staging table is cleaned up by dropping vcluster_raw when done (use DROP TABLE IF EXISTS vcluster_raw after the INSERT) and optionally defensively drop it before CREATE (DROP TABLE IF EXISTS vcluster_raw) to avoid CREATE conflicts on repeated runs.
🤖 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.
Outside diff comments:
In `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl`:
- Around line 282-301: The staging table vcluster_raw is created but never
removed, causing subsequent ingests to fail or leave stale data; update the
template around the vcluster_raw lifecycle (the CREATE TABLE vcluster_raw ...
and subsequent INSERT INTO vcluster ...) to ensure the staging table is cleaned
up by dropping vcluster_raw when done (use DROP TABLE IF EXISTS vcluster_raw
after the INSERT) and optionally defensively drop it before CREATE (DROP TABLE
IF EXISTS vcluster_raw) to avoid CREATE conflicts on repeated runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb80d554-bddd-43b6-a3b5-c0e3e50119cb
📒 Files selected for processing (14)
api/v1alpha1/agent/spec.gen.goapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gopkg/duckdb_parser/builder.gopkg/duckdb_parser/inventory_builder.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/models/inventory.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/templates/cluster_features_query.go.tmplpkg/duckdb_parser/templates/create_schema.go.tmplpkg/duckdb_parser/templates/ingest_rvtools.go.tmplpkg/inventory/converters/to_api.gopkg/inventory/model.go
itroyano
force-pushed
the
ECOPROJECT-4282
branch
2 times, most recently
from
c284cb2 to
a57f743
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl (1)
282-301:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
vcluster_rawafter insert to prevent re-ingest failures.
vcluster_rawis created on Line 282 but never cleaned up. A second ingest in the same DB will fail onCREATE TABLE vcluster_raw AS ...and stale staging data can persist.🔧 Proposed fix
INSERT INTO vcluster ("Name", "Object ID", "DrsEnabled", "DrsDefaultVmBehavior", "StorageDrsEnabled") SELECT "Name", "Object ID", CASE WHEN LOWER(TRIM(COALESCE("drs", ''))) IN ('true', '1', 'yes', 'enabled') THEN true ELSE false END as "DrsEnabled", COALESCE(NULLIF(TRIM("DrsDefaultVmBehavior"), ''), 'None') as "DrsDefaultVmBehavior", CASE WHEN LOWER(TRIM(COALESCE("Storage DRS", ''))) IN ('true', '1', 'yes', 'enabled') THEN true ELSE false END as "StorageDrsEnabled" FROM vcluster_raw; + +DROP TABLE vcluster_raw;🤖 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 `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl` around lines 282 - 301, The template creates a temporary staging table vcluster_raw via read_xlsx and never drops it, causing subsequent runs to fail; after the INSERT INTO vcluster (...) SELECT ... FROM vcluster_raw; add a DROP TABLE IF EXISTS vcluster_raw; (or DROP TABLE vcluster_raw) to clean up the staging table so repeated ingests succeed — modify the template around the INSERT INTO vcluster (the vcluster_raw staging usage) to drop vcluster_raw after the insert.
🤖 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 `@pkg/duckdb_parser/inventory_builder.go`:
- Around line 123-129: The code currently swallows every error from
p.ClusterFeatures(ctx, clusterName) by returning nil, nil; change this so only
known non-fatal cases (e.g., a specific "not found" or "feature unsupported"
sentinel/error type returned by ClusterFeatures) are treated as
backward-compatible and allow continuing, while all other errors are wrapped
with context and returned; locate the call to p.ClusterFeatures in
inventory_builder.go (variables clusterFeatures, err) and replace the
unconditional return nil, nil with logic that checks the error type/value, logs
appropriately via zap.S().Named("duckdb_parser"), and returns
fmt.Errorf("retrieving cluster features for %s: %w", clusterName, err) (or the
project’s error-wrapping helper) for non-backward-compatible failures.
In `@pkg/duckdb_parser/queries.go`:
- Around line 532-534: The current return replaces sql.ErrNoRows with a new
formatted error, losing the original error classification; change the branch
that checks errors.Is(err, sql.ErrNoRows) to wrap the original err instead of
discarding it (e.g., return nil, fmt.Errorf("cluster %q not found: %w",
clusterName, err)) so callers can still detect sql.ErrNoRows via errors.Is;
update the block around the error check in pkg/duckdb_parser/queries.go where
clusterName and err are used.
---
Outside diff comments:
In `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl`:
- Around line 282-301: The template creates a temporary staging table
vcluster_raw via read_xlsx and never drops it, causing subsequent runs to fail;
after the INSERT INTO vcluster (...) SELECT ... FROM vcluster_raw; add a DROP
TABLE IF EXISTS vcluster_raw; (or DROP TABLE vcluster_raw) to clean up the
staging table so repeated ingests succeed — modify the template around the
INSERT INTO vcluster (the vcluster_raw staging usage) to drop vcluster_raw after
the insert.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5ad7bd05-f019-4789-84e1-3702f9832751
📒 Files selected for processing (14)
api/v1alpha1/agent/spec.gen.goapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gopkg/duckdb_parser/builder.gopkg/duckdb_parser/inventory_builder.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/models/inventory.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/templates/cluster_features_query.go.tmplpkg/duckdb_parser/templates/create_schema.go.tmplpkg/duckdb_parser/templates/ingest_rvtools.go.tmplpkg/inventory/converters/to_api.gopkg/inventory/model.go
Comment on lines
+123
to
+129
| clusterFeatures, err := p.ClusterFeatures(ctx, clusterName) | ||
| if err != nil { | ||
| // For backward compatibility, if cluster features aren't available, | ||
| // continue without them rather than failing the entire inventory | ||
| zap.S().Named("duckdb_parser").Debugf("Cluster features not available for %s: %v", clusterName, err) | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Do not swallow all cluster-feature retrieval errors.
Line 128 converts every failure into success (nil, nil). That hides DB/query failures and makes troubleshooting hard; only true “not found/backward-compatible” cases should be non-fatal.
Suggested fix
import (
"context"
"crypto/sha256"
+ "database/sql"
+ "errors"
"fmt"
@@
func (p *Parser) buildClusterData(ctx context.Context, clusterName string) (*models.Cluster, error) {
clusterFeatures, err := p.ClusterFeatures(ctx, clusterName)
if err != nil {
- // For backward compatibility, if cluster features aren't available,
- // continue without them rather than failing the entire inventory
- zap.S().Named("duckdb_parser").Debugf("Cluster features not available for %s: %v", clusterName, err)
- return nil, nil
+ if errors.Is(err, sql.ErrNoRows) {
+ zap.S().Named("duckdb_parser").Debugf("Cluster features not available for %s: %v", clusterName, err)
+ return nil, nil
+ }
+ return nil, fmt.Errorf("getting cluster features for %s: %w", clusterName, err)
}As per coding guidelines "Error Handling: No silent failures. Errors must be checked, wrapped with context, and propagated."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| clusterFeatures, err := p.ClusterFeatures(ctx, clusterName) | |
| if err != nil { | |
| // For backward compatibility, if cluster features aren't available, | |
| // continue without them rather than failing the entire inventory | |
| zap.S().Named("duckdb_parser").Debugf("Cluster features not available for %s: %v", clusterName, err) | |
| return nil, nil | |
| } | |
| clusterFeatures, err := p.ClusterFeatures(ctx, clusterName) | |
| if err != nil { | |
| if errors.Is(err, sql.ErrNoRows) { | |
| zap.S().Named("duckdb_parser").Debugf("Cluster features not available for %s: %v", clusterName, err) | |
| return nil, nil | |
| } | |
| return nil, fmt.Errorf("getting cluster features for %s: %w", clusterName, err) | |
| } |
🤖 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 `@pkg/duckdb_parser/inventory_builder.go` around lines 123 - 129, The code
currently swallows every error from p.ClusterFeatures(ctx, clusterName) by
returning nil, nil; change this so only known non-fatal cases (e.g., a specific
"not found" or "feature unsupported" sentinel/error type returned by
ClusterFeatures) are treated as backward-compatible and allow continuing, while
all other errors are wrapped with context and returned; locate the call to
p.ClusterFeatures in inventory_builder.go (variables clusterFeatures, err) and
replace the unconditional return nil, nil with logic that checks the error
type/value, logs appropriately via zap.S().Named("duckdb_parser"), and returns
fmt.Errorf("retrieving cluster features for %s: %w", clusterName, err) (or the
project’s error-wrapping helper) for non-backward-compatible failures.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl (1)
290-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
vcluster_rawafter the insert to keep ingest idempotent.
vcluster_rawis created but never removed. A subsequent ingest in the same DB can fail when recreating this table.🔧 Proposed fix
INSERT INTO vcluster ("Name", "Object ID", "DrsEnabled", "DrsDefaultVmBehavior", "StorageDrsEnabled") SELECT "Name", "Object ID", CASE WHEN LOWER(TRIM(COALESCE("drs", ''))) IN ('true', '1', 'yes', 'enabled') THEN true ELSE false END as "DrsEnabled", COALESCE(NULLIF(TRIM("DrsDefaultVmBehavior"), ''), 'None') as "DrsDefaultVmBehavior", CASE WHEN LOWER(TRIM(COALESCE("Storage DRS", ''))) IN ('true', '1', 'yes', 'enabled') THEN true ELSE false END as "StorageDrsEnabled" FROM vcluster_raw; + +DROP TABLE vcluster_raw;🤖 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 `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl` around lines 290 - 309, After inserting from the temporary staging table vcluster_raw, drop that table to make the ingest idempotent; specifically, add a DROP TABLE IF EXISTS vcluster_raw; statement immediately after the INSERT INTO vcluster (...) SELECT ... FROM vcluster_raw; block so subsequent runs can recreate vcluster_raw without error and avoid leftover staging data.
♻️ Duplicate comments (2)
pkg/duckdb_parser/queries.go (1)
535-537:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the not-found error instead of replacing it.
The current branch drops
sql.ErrNoRows, so callers cannot reliably distinguish “not found” from other failures.🔧 Proposed fix
if err != nil { if errors.Is(err, sql.ErrNoRows) { - return nil, fmt.Errorf("cluster %q not found", clusterName) + return nil, fmt.Errorf("cluster %q not found: %w", clusterName, err) } return nil, fmt.Errorf("scanning cluster features: %w", err) }As per coding guidelines "Error Handling: No silent failures. Errors must be checked, wrapped with context, and propagated."
🤖 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 `@pkg/duckdb_parser/queries.go` around lines 535 - 537, The branch that detects errors.Is(err, sql.ErrNoRows) should wrap the original sql.ErrNoRows instead of replacing it so callers can still detect the sentinel; update the handler in pkg/duckdb_parser/queries.go (the cluster lookup that checks errors.Is(err, sql.ErrNoRows) with clusterName and err) to return a wrapped error containing context (e.g., "cluster %q not found") while preserving the original err via %w (or errors.Join) so errors.Is(err, sql.ErrNoRows) continues to work.pkg/duckdb_parser/inventory_builder.go (1)
132-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly swallow true backward-compatible misses, not all errors.
Returning
nil, nilfor everyClusterFeaturesfailure hides real query/DB problems and makes troubleshooting harder.🔧 Proposed fix
import ( "context" "crypto/sha256" + "database/sql" + "errors" "fmt" @@ func (p *Parser) buildClusterData(ctx context.Context, clusterName string) (*models.Cluster, error) { clusterFeatures, err := p.ClusterFeatures(ctx, clusterName) if err != nil { - // For backward compatibility, if cluster features aren't available, - // continue without them rather than failing the entire inventory - zap.S().Named("duckdb_parser").Debugf("Cluster features not available for %s: %v", clusterName, err) - return nil, nil + if errors.Is(err, sql.ErrNoRows) { + zap.S().Named("duckdb_parser").Debugf("Cluster features not available for %s: %v", clusterName, err) + return nil, nil + } + return nil, fmt.Errorf("retrieving cluster features for %s: %w", clusterName, err) }As per coding guidelines "Error Handling: No silent failures. Errors must be checked, wrapped with context, and propagated."
🤖 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 `@pkg/duckdb_parser/inventory_builder.go` around lines 132 - 137, The current error handling around the ClusterFeatures retrieval swallows all errors (returning nil, nil) which hides real failures; change the logic in the inventory_builder.go block that checks "if err != nil" to only treat a known backward-compatible sentinel/feature-unavailable error as non-fatal (using errors.Is against a specific sentinel like ErrClusterFeaturesNotSupported or ErrFeatureUnavailable) and keep the existing debug log + return nil, nil for that case; for any other error, wrap it with context (including clusterName) and return it (e.g., return nil, fmt.Errorf("fetching ClusterFeatures for %s: %w", clusterName, err)) so callers can detect real DB/query failures. Ensure you reference the same zap logger (zap.S().Named("duckdb_parser")) and use errors.Is to compare the error rather than string matching.
🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 1527-1535: The test captures pointers to the loop range variable
(cluster), causing pointer aliasing so drsEnabledCluster, drsDisabledCluster,
and noDrsDataCluster all end up pointing to the same value; fix by avoiding
taking &cluster inside the range: either look up each cluster directly from the
map (e.g., use inv.Clusters["domain-c1"], inv.Clusters["domain-c2"],
inv.Clusters["domain-c3"]) or create a local copy inside the loop (c := cluster;
use &c) so each pointer (drsEnabledCluster, drsDisabledCluster,
noDrsDataCluster) references a distinct struct; apply the same change for the
other similar block that populates those variables.
---
Outside diff comments:
In `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl`:
- Around line 290-309: After inserting from the temporary staging table
vcluster_raw, drop that table to make the ingest idempotent; specifically, add a
DROP TABLE IF EXISTS vcluster_raw; statement immediately after the INSERT INTO
vcluster (...) SELECT ... FROM vcluster_raw; block so subsequent runs can
recreate vcluster_raw without error and avoid leftover staging data.
---
Duplicate comments:
In `@pkg/duckdb_parser/inventory_builder.go`:
- Around line 132-137: The current error handling around the ClusterFeatures
retrieval swallows all errors (returning nil, nil) which hides real failures;
change the logic in the inventory_builder.go block that checks "if err != nil"
to only treat a known backward-compatible sentinel/feature-unavailable error as
non-fatal (using errors.Is against a specific sentinel like
ErrClusterFeaturesNotSupported or ErrFeatureUnavailable) and keep the existing
debug log + return nil, nil for that case; for any other error, wrap it with
context (including clusterName) and return it (e.g., return nil,
fmt.Errorf("fetching ClusterFeatures for %s: %w", clusterName, err)) so callers
can detect real DB/query failures. Ensure you reference the same zap logger
(zap.S().Named("duckdb_parser")) and use errors.Is to compare the error rather
than string matching.
In `@pkg/duckdb_parser/queries.go`:
- Around line 535-537: The branch that detects errors.Is(err, sql.ErrNoRows)
should wrap the original sql.ErrNoRows instead of replacing it so callers can
still detect the sentinel; update the handler in pkg/duckdb_parser/queries.go
(the cluster lookup that checks errors.Is(err, sql.ErrNoRows) with clusterName
and err) to return a wrapped error containing context (e.g., "cluster %q not
found") while preserving the original err via %w (or errors.Join) so
errors.Is(err, sql.ErrNoRows) continues to work.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9324e7d0-a72e-4570-9b9c-f5bb90d85423
📒 Files selected for processing (14)
api/v1alpha1/agent/spec.gen.goapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gopkg/duckdb_parser/builder.gopkg/duckdb_parser/inventory_builder.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/models/inventory.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/templates/cluster_features_query.go.tmplpkg/duckdb_parser/templates/create_schema.go.tmplpkg/duckdb_parser/templates/ingest_rvtools.go.tmplpkg/inventory/converters/to_api.gopkg/inventory/model.go
Comment on lines
+1527
to
+1535
| for clusterID, cluster := range inv.Clusters { | ||
| switch clusterID { | ||
| case "domain-c1": // cluster-drs-enabled | ||
| drsEnabledCluster = &cluster | ||
| case "domain-c2": // cluster-drs-disabled | ||
| drsDisabledCluster = &cluster | ||
| case "domain-c3": // cluster-no-drs-data | ||
| noDrsDataCluster = &cluster | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify range-variable address capture patterns in this test file.
rg -n -C2 'for .*range inv\.Clusters|&cluster' pkg/duckdb_parser/inventory_builder_test.goRepository: kubev2v/migration-planner
Length of output: 1982
Fix pointer aliasing when capturing clusters from map iteration.
Taking the address of the range variable at lines 1530, 1532, and 1534 causes all captured pointers to reference the same memory location. After the loop, they all point to the final value of cluster, making the DRS assertions validate the wrong cluster data.
Instead, directly access the map by key to avoid the aliasing:
Suggested fix
- // Find each cluster and verify their DRS settings
- var drsEnabledCluster, drsDisabledCluster, noDrsDataCluster *inventory.InventoryData
- for clusterID, cluster := range inv.Clusters {
- switch clusterID {
- case "domain-c1": // cluster-drs-enabled
- drsEnabledCluster = &cluster
- case "domain-c2": // cluster-drs-disabled
- drsDisabledCluster = &cluster
- case "domain-c3": // cluster-no-drs-data
- noDrsDataCluster = &cluster
- }
- }
+ // Fetch each cluster by ID directly to avoid range-variable pointer aliasing.
+ drsEnabledCluster, ok := inv.Clusters["domain-c1"] // cluster-drs-enabled
+ require.True(t, ok, "Should find cluster-drs-enabled")
+ drsDisabledCluster, ok := inv.Clusters["domain-c2"] // cluster-drs-disabled
+ require.True(t, ok, "Should find cluster-drs-disabled")
+ noDrsDataCluster, ok := inv.Clusters["domain-c3"] // cluster-no-drs-data
+ require.True(t, ok, "Should find cluster-no-drs-data")Same issue applies to the similar code block at lines 1539–1554.
🤖 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 `@pkg/duckdb_parser/inventory_builder_test.go` around lines 1527 - 1535, The
test captures pointers to the loop range variable (cluster), causing pointer
aliasing so drsEnabledCluster, drsDisabledCluster, and noDrsDataCluster all end
up pointing to the same value; fix by avoiding taking &cluster inside the range:
either look up each cluster directly from the map (e.g., use
inv.Clusters["domain-c1"], inv.Clusters["domain-c2"], inv.Clusters["domain-c3"])
or create a local copy inside the loop (c := cluster; use &c) so each pointer
(drsEnabledCluster, drsDisabledCluster, noDrsDataCluster) references a distinct
struct; apply the same change for the other similar block that populates those
variables.
Signed-off-by: Igor Troyanovsky <itroyano@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
nirarg
approved these changes
May 19, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirarg The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Collaborator
|
/ok-to-test |
Collaborator
|
/retest |
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
2 participants
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.
Signed-off-by: Igor Troyanovsky itroyano@redhat.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Summary by CodeRabbit
New Features
Tests