ECOPROJECT-4328 | fix: require VI SDK UUID and fix inventory snapshot version detection#1035
Conversation
ronenav
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:
📝 WalkthroughWalkthroughGetInventoryVersion now detects snapshot V2 by checking top-level JSON keys Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
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/queries.go (1)
430-444:⚠️ Potential issue | 🟡 MinorAdd a comment explaining why
sql.ErrNoRowsis treated as non-fatal.This change silently converts
sql.ErrNoRowsto a successful empty-string return, eliminating the warning log that previously triggered on missing rows. While the caller (BuildInventory) handles both outcomes identically, the semantic shift reduces observability without explanation. The schema validation inschema_validation.goalready rejects files with missing or empty VI SDK UUID values, makingErrNoRowsan edge case that structurally shouldn't occur at this point. Add a brief comment explaining this assumption so future maintainers understand why the error is intentionally suppressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/duckdb_parser/queries.go` around lines 430 - 444, In Parser.VCenterID, add a brief explanatory comment at the sql.ErrNoRows branch (the spot that returns "", nil) stating that sql.ErrNoRows is intentionally treated as non-fatal because callers such as BuildInventory treat empty vCenter IDs the same as missing ones and upstream schema_validation.go already rejects files with missing or empty VI SDK UUIDs, so ErrNoRows is an unexpected edge case that is suppressed for observability reasons rather than flow control; mention that this comment documents the assumption and where to look (schema_validation.go) if the invariant is violated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/util/util_test.go`:
- Around line 9-50: Add tests to internal/util/util_test.go that assert
GetInventoryVersion([]byte(...)) falls back to model.SnapshotVersionV1 for
error/empty inputs: include cases for malformed JSON (e.g. truncated or
invalid), empty input ("" and []byte{}), and JSON null ("null"), each as new
entries in the existing tests slice (referencing GetInventoryVersion and
model.SnapshotVersionV1) and run them in the same t.Run/t.Parallel pattern so
the fallback behavior is verified.
In `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 503-506: Add a unit test case to cover whitespace-only VI SDK
UUIDs by inserting a table entry in the existing test slice (the same place as
the case named "empty VI SDK UUID values report MISSING_VI_SDK_UUID") where the
map for the VM sets "VI SDK UUID" to a string of spaces (e.g. " "); ensure
this new case calls validateVinfoData (the same test harness) and expects
CodeMissingVISDKUUID so that trimmed-whitespace is treated as empty.
In `@pkg/duckdb_parser/schema_validation.go`:
- Around line 143-151: The check using p.hasValidColumnData currently treats any
DB/query error as "no data" and reports CodeMissingVISDKUUID; change
hasValidColumnData to return (bool, error) (or otherwise expose the underlying
query error from countRows), then update callers (the p.hasValidColumnData call
for "vinfo_raw" and the other two similar branches) to check the error: if err
!= nil append a ValidationIssue that records a DB/query failure (wrap the error
with context and include it in Message) and do not mark it as
CodeMissingVISDKUUID, otherwise proceed based on the boolean result; ensure
errors are wrapped with context before being added to result.Errors so query
failures are surfaced instead of being collapsed into missing-column issues.
---
Outside diff comments:
In `@pkg/duckdb_parser/queries.go`:
- Around line 430-444: In Parser.VCenterID, add a brief explanatory comment at
the sql.ErrNoRows branch (the spot that returns "", nil) stating that
sql.ErrNoRows is intentionally treated as non-fatal because callers such as
BuildInventory treat empty vCenter IDs the same as missing ones and upstream
schema_validation.go already rejects files with missing or empty VI SDK UUIDs,
so ErrNoRows is an unexpected edge case that is suppressed for observability
reasons rather than flow control; mention that this comment documents the
assumption and where to look (schema_validation.go) if the invariant is
violated.
🪄 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: 96359f85-2e4a-41d3-aadf-3c4cdcbea008
📒 Files selected for processing (5)
internal/util/util.gointernal/util/util_test.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/schema_validation.go
ronenav
force-pushed
the
4328-missing-vcenter-id
branch
2 times, most recently
from
064e94f to
79cce7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/duckdb_parser/inventory_builder_test.go (1)
495-507: 🧹 Nitpick | 🔵 TrivialTighten the
VI SDK UUIDregression cases.These cases only assert that
MISSING_VI_SDK_UUIDis present. They still pass if validation also emitsCOLUMN_VALIDATION_FAILED, so they won’t catch the generic-error regression this PR is trying to prevent. They also still miss the whitespace-only path thatTRIM(...) != ''treats as empty.As per coding guidelines, "Coverage: Strive for high test coverage on critical logic paths."✅ Suggested test additions
{ name: "missing VI SDK UUID column reports MISSING_VI_SDK_UUID", customHeaders: []string{"VM", "VM ID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"}, vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster1", "Datacenter": "dc1"}}, expectedCodes: []string{CodeMissingVISDKUUID}, - forbiddenCodes: []string{CodeNoVMs}, + forbiddenCodes: []string{CodeNoVMs, CodeColumnValidationFailed}, }, { name: "empty VI SDK UUID values report MISSING_VI_SDK_UUID", customHeaders: []string{"VM", "VM ID", "VI SDK UUID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"}, vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "VI SDK UUID": "", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster1", "Datacenter": "dc1"}}, expectedCodes: []string{CodeMissingVISDKUUID}, - forbiddenCodes: []string{CodeNoVMs}, + forbiddenCodes: []string{CodeNoVMs, CodeColumnValidationFailed}, + }, + { + name: "whitespace VI SDK UUID values report MISSING_VI_SDK_UUID", + customHeaders: []string{"VM", "VM ID", "VI SDK UUID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"}, + vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "VI SDK UUID": " ", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster1", "Datacenter": "dc1"}}, + expectedCodes: []string{CodeMissingVISDKUUID}, + forbiddenCodes: []string{CodeNoVMs, CodeColumnValidationFailed}, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/duckdb_parser/inventory_builder_test.go` around lines 495 - 507, Tighten the two VI SDK UUID tests in inventory_builder_test.go: for the case with the header missing and the case with an empty value, assert not only that CodeMissingVISDKUUID is present but also that CodeColumnValidationFailed (COLUMN_VALIDATION_FAILED) is NOT present (use forbiddenCodes to include CodeColumnValidationFailed), and add an additional subcase for a whitespace-only VI SDK UUID value (e.g., " ") to ensure the TRIM path is exercised; reference the test case entries that use expectedCodes/forbiddenCodes and the constants CodeMissingVISDKUUID and CodeNoVMs to locate where to modify/add these assertions.pkg/duckdb_parser/schema_validation.go (1)
169-172:⚠️ Potential issue | 🟠 MajorDon’t drop optional-table validation failures on the floor.
This
continuemakes a broken optional-table count query indistinguishable from “no warning was needed”, so host/datastore/network sections can disappear with no validation signal explaining why.internal/rvtools/jobs/worker.go:109-123already logs warnings by code/message, so surfacing a warning issue here would stay non-fatal while restoring diagnosability.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 the current code and only fix it if needed. In `@pkg/duckdb_parser/schema_validation.go` around lines 169 - 172, The optional-table count error is being swallowed by the bare "continue"; instead wrap the error with context (e.g. fmt.Errorf("count rows for table %s: %w", check.table, err)) and surface it as a non-fatal validation warning using the repository's validation-warning mechanism (e.g. append to the parser/validation warnings slice or call the existing add/emit warning helper such as p.addWarning or p.emitValidationWarning with a code/message and the wrapped error) before continuing; do this in the code that calls p.countRowsErr so the failure is recorded (not dropped) while remaining non-fatal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 768-779: The test TestBuildInventory_MinimalSchema currently only
checks validation and Total == 3 but doesn't assert the vCenter identifier;
after calling BuildInventory (or whatever returns the built inventory in this
test) add an assertion that the built inventory's vCenter/VI SDK UUID field
equals the local vcUUID variable (e.g., assert.Equal(t, vcUUID,
inventory.VCenterID) or the correct field name used by BuildInventory) so the
parsed VI SDK UUID is preserved.
In `@pkg/duckdb_parser/queries.go`:
- Around line 437-440: The helper currently converts sql.ErrNoRows into a nil
error by returning ("", nil) after p.db.QueryRowContext(ctx,
q).Scan(&vcenterID), which hides missing-row conditions; change this to
propagate the error (return "", err) and wrap it with context (e.g., using
fmt.Errorf("fetch vcenter id: %w", err)) so callers can detect and downgrade
sql.ErrNoRows if appropriate; locate the QueryRowContext(...).Scan(&vcenterID)
call in queries.go and replace the ("", nil) return with a propagated, wrapped
error.
---
Duplicate comments:
In `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 495-507: Tighten the two VI SDK UUID tests in
inventory_builder_test.go: for the case with the header missing and the case
with an empty value, assert not only that CodeMissingVISDKUUID is present but
also that CodeColumnValidationFailed (COLUMN_VALIDATION_FAILED) is NOT present
(use forbiddenCodes to include CodeColumnValidationFailed), and add an
additional subcase for a whitespace-only VI SDK UUID value (e.g., " ") to
ensure the TRIM path is exercised; reference the test case entries that use
expectedCodes/forbiddenCodes and the constants CodeMissingVISDKUUID and
CodeNoVMs to locate where to modify/add these assertions.
In `@pkg/duckdb_parser/schema_validation.go`:
- Around line 169-172: The optional-table count error is being swallowed by the
bare "continue"; instead wrap the error with context (e.g. fmt.Errorf("count
rows for table %s: %w", check.table, err)) and surface it as a non-fatal
validation warning using the repository's validation-warning mechanism (e.g.
append to the parser/validation warnings slice or call the existing add/emit
warning helper such as p.addWarning or p.emitValidationWarning with a
code/message and the wrapped error) before continuing; do this in the code that
calls p.countRowsErr so the failure is recorded (not dropped) while remaining
non-fatal.
🪄 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: 1977ba21-68cf-4fd9-b945-3af820ad1ae3
📒 Files selected for processing (5)
internal/util/util.gointernal/util/util_test.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/schema_validation.go
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/inventory_builder_test.go (1)
775-799: 🧹 Nitpick | 🔵 TrivialAssert that the built inventory preserves the vCenter UUID.
The test defines
vcUUIDand uses it in the Excel data, but only verifies that validation passes andTotal == 3. Adding an assertion oninv.VCenterIDwould catch regressions where the parsed VI SDK UUID isn't correctly propagated to the inventory.💡 Suggested addition
inv, err := parser.BuildInventory(ctx) require.NoError(t, err) assert.Equal(t, 3, inv.VCenter.VMs.Total, "Should have 3 VMs from minimal schema") + assert.Equal(t, vcUUID, inv.VCenterID, "VCenterID should be populated from VI SDK UUID column")As per coding guidelines, "Coverage: Strive for high test coverage on critical logic paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/duckdb_parser/inventory_builder_test.go` around lines 775 - 799, The test TestBuildInventory_MinimalSchema sets vcUUID in the input but never asserts it is propagated into the built inventory; update the test to assert that after parser.IngestRvTools and parser.BuildInventory the resulting inv.VCenterID (or the appropriate field on inv.VCenter) equals vcUUID so regressions in VI SDK UUID parsing are caught—locate TestBuildInventory_MinimalSchema, use the existing vcUUID variable and add a require.Equal/ assert.Equal check comparing vcUUID to inv.VCenterID (or inv.VCenter.ID) immediately after inv is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/duckdb_parser/schema_validation.go`:
- Around line 169-172: When countRowsErr returns an error in the optional table
validation loop, don’t silently ignore it — log the failure with the table name
and error before continuing. Update the block that calls p.countRowsErr (using
check.table) to emit a debug/info log (e.g., p.logger.Infof or p.logger.Debugf,
or fallback to log.Printf if no logger is available) including the table name
and err (and optionally the query) and then continue.
---
Outside diff comments:
In `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 775-799: The test TestBuildInventory_MinimalSchema sets vcUUID in
the input but never asserts it is propagated into the built inventory; update
the test to assert that after parser.IngestRvTools and parser.BuildInventory the
resulting inv.VCenterID (or the appropriate field on inv.VCenter) equals vcUUID
so regressions in VI SDK UUID parsing are caught—locate
TestBuildInventory_MinimalSchema, use the existing vcUUID variable and add a
require.Equal/ assert.Equal check comparing vcUUID to inv.VCenterID (or
inv.VCenter.ID) immediately after inv is returned.
🪄 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: 4bd97a5a-fd7d-4696-911b-c2a7f36b2a1a
📒 Files selected for processing (5)
internal/util/util.gointernal/util/util_test.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/schema_validation.go
ronenav
force-pushed
the
4328-missing-vcenter-id
branch
2 times, most recently
from
8c55da1 to
44ca732
Compare
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/inventory_builder_test.go (1)
775-799: 🧹 Nitpick | 🔵 TrivialAdd assertion that built inventory contains the expected vCenter ID.
The test feeds
vcUUIDinto the minimal schema but only asserts validation success and VM count. A regression whereBuildInventoryignores or fails to parse the VI SDK UUID column would still pass this test.💡 Suggested addition
inv, err := parser.BuildInventory(ctx) require.NoError(t, err) assert.Equal(t, 3, inv.VCenter.VMs.Total, "Should have 3 VMs from minimal schema") + assert.Equal(t, vcUUID, inv.VCenterID, "VCenter ID should be populated from VI SDK UUID")As per coding guidelines, "Coverage: Strive for high test coverage on critical logic paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/duckdb_parser/inventory_builder_test.go` around lines 775 - 799, Add an assertion to TestBuildInventory_MinimalSchema that verifies the vCenter ID from the built inventory matches the input vcUUID so regressions that drop/ignore the "VI SDK UUID" column are caught; after calling parser.BuildInventory(ctx) and checking inv.VCenter.VMs.Total, assert that inv.VCenter.ID (or the field representing the vCenter UUID on the returned inventory struct) equals the local vcUUID used when creating the test XLSX, failing the test if it does not match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/util/util.go`:
- Around line 157-158: The code currently swallows JSON unmarshal errors by
returning model.SnapshotVersionV1 when json.Unmarshal(inventory, &keys) fails;
change the version-detection routine to return an explicit error (or a distinct
sentinel like model.SnapshotVersionUnknown) instead of silently returning
SnapshotVersionV1, update its signature to (model.SnapshotVersion, error), wrap
the json.Unmarshal error with context, and propagate the error to call sites
(e.g., where Snapshot.Version is set in internal/store/assessment.go) so callers
can decide whether to treat it as a downgrade, reject the payload, or handle
recovery.
In `@pkg/duckdb_parser/schema_validation.go`:
- Around line 200-210: In isTableMissingError, remove the incorrect check for
sql.ErrNoRows (dead code because COUNT(*) always returns one row) and instead
ensure you first guard against a nil error (if err == nil return false) before
performing the string-match logic; update the function (isTableMissingError)
used by countRowsErr to no longer test errors.Is(err, sql.ErrNoRows) and rely
solely on lowercased err.Error() pattern checks for "table" + ("not found" |
"does not exist" | "no such table").
---
Outside diff comments:
In `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 775-799: Add an assertion to TestBuildInventory_MinimalSchema that
verifies the vCenter ID from the built inventory matches the input vcUUID so
regressions that drop/ignore the "VI SDK UUID" column are caught; after calling
parser.BuildInventory(ctx) and checking inv.VCenter.VMs.Total, assert that
inv.VCenter.ID (or the field representing the vCenter UUID on the returned
inventory struct) equals the local vcUUID used when creating the test XLSX,
failing the test if it does not match.
🪄 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: 611b4d72-2aa3-48b8-b35d-e1ad4afa87ac
📒 Files selected for processing (5)
internal/util/util.gointernal/util/util_test.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/schema_validation.go
ronenav
force-pushed
the
4328-missing-vcenter-id
branch
from
44ca732 to
20d9458
Compare
nirarg
left a comment
nirarg
left a comment
There was a problem hiding this comment.
1 comment about function name
I leave the decision up to you
everything else looks good
ronenav
force-pushed
the
4328-missing-vcenter-id
branch
from
20d9458 to
680679b
Compare
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/inventory_builder_test.go (1)
857-938: 🧹 Nitpick | 🔵 TrivialTest data consistency note.
The
TestBuildInventory_ComplexityDistributionWithDiskSizetest does not include"VI SDK UUID"in its VM data (lines 866-878), but this test relies ondefaultStandardSheetswhich usesvInfoHeaders(line 85-92) that includes"VI SDK UUID". Since the VM maps don't provide this key, the cells will be empty.However, this test manually updates the DB after ingestion and focuses on complexity distribution, not VI SDK UUID validation. The test may still pass because:
- It doesn't call
ValidateSchemawith"vinfo_raw"tabledefaultStandardSheetsuses the fullvInfoHeaderswhich includes the columnConsider adding
"VI SDK UUID"to these VM maps for consistency with other tests, though this is not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/duckdb_parser/inventory_builder_test.go` around lines 857 - 938, The test TestBuildInventory_ComplexityDistributionWithDiskSize uses defaultStandardSheets which expects vInfoHeaders to include "VI SDK UUID", but the VM map entries omit that key; add "VI SDK UUID" with an appropriate value (e.g., a unique string) to each VM map in the vms slice so the input rows match vInfoHeaders and stay consistent with other tests and defaultStandardSheets/ vInfoHeaders usage.
♻️ Duplicate comments (1)
pkg/duckdb_parser/schema_validation.go (1)
198-205: 🧹 Nitpick | 🔵 TrivialAdd nil guard for defensive coding.
While callers currently ensure
err != nilbefore callingisTableMissingError, adding an explicit nil check would make the function safer against future refactoring.🛡️ Defensive nil check
func isTableMissingError(err error) bool { + if err == nil { + return false + } errMsg := strings.ToLower(err.Error()) return strings.Contains(errMsg, "table") && (strings.Contains(errMsg, "not found") || strings.Contains(errMsg, "does not exist") || strings.Contains(errMsg, "no such table")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/duckdb_parser/schema_validation.go` around lines 198 - 205, The isTableMissingError function should defensively handle a nil error: add an explicit nil check at the start of isTableMissingError(err error) that returns false if err == nil, then proceed to lowercase err.Error() and the existing substring checks; update the function body around isTableMissingError to avoid calling err.Error() when err is nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/util/util_test.go`:
- Around line 9-65: Add a test case to TestGetInventoryVersion to cover the
empty JSON object scenario: call GetInventoryVersion with []byte("{}") and
assert it returns model.SnapshotVersionV1; update the tests slice in
internal/util/util_test.go (the table in TestGetInventoryVersion) by adding a
case named like "empty object defaults to v1" so GetInventoryVersion is verified
to treat an empty JSON object as legacy v1.
---
Outside diff comments:
In `@pkg/duckdb_parser/inventory_builder_test.go`:
- Around line 857-938: The test
TestBuildInventory_ComplexityDistributionWithDiskSize uses defaultStandardSheets
which expects vInfoHeaders to include "VI SDK UUID", but the VM map entries omit
that key; add "VI SDK UUID" with an appropriate value (e.g., a unique string) to
each VM map in the vms slice so the input rows match vInfoHeaders and stay
consistent with other tests and defaultStandardSheets/ vInfoHeaders usage.
---
Duplicate comments:
In `@pkg/duckdb_parser/schema_validation.go`:
- Around line 198-205: The isTableMissingError function should defensively
handle a nil error: add an explicit nil check at the start of
isTableMissingError(err error) that returns false if err == nil, then proceed to
lowercase err.Error() and the existing substring checks; update the function
body around isTableMissingError to avoid calling err.Error() when err is nil.
🪄 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: 9ce039cd-943a-45c8-a38f-a994b1c61ae0
📒 Files selected for processing (5)
internal/util/util.gointernal/util/util_test.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/queries.gopkg/duckdb_parser/schema_validation.go
… version detection Signed-off-by: Ronen Avraham <ravraham@redhat.com>
ronenav
force-pushed
the
4328-missing-vcenter-id
branch
from
680679b to
a3df180
Compare
|
[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 |
2 participants
This PR addresses the issue where RVTools Excel files missing the "VI SDK UUID" column (or with empty values) would result in empty assessment reports.
The fix adds proper validation to reject such files early with a clear error message, and fixes a bug in inventory version detection.
Summary by CodeRabbit
New Features
Bug Fixes
Tests