ECOPROJECT-4839 | feat: add collection_id to the duck_db ingestion logic - #1304
ECOPROJECT-4839 | feat: add collection_id to the duck_db ingestion logic#1304amalimov wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughIntroduces collection-aware SQLite ingestion. ChangesCollection-aware SQLite ingestion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant QueryBuilder
participant Template
participant Parser as IngestSqliteWithCollection
participant DB as SQLite/DuckDB
Caller->>Parser: IngestSqliteWithCollection(sqliteFile, collectionID)
Parser->>Parser: validate collectionID >= 0
Parser->>QueryBuilder: IngestSqliteQueryWithCollection(filePath, collectionID)
QueryBuilder->>Template: build ingest_sqlite with CollectionID
Template->>Template: compute md5(collectionID_v.ID) as VM ID, filter by vmmoid
Template-->>QueryBuilder: generated SQL
QueryBuilder-->>Parser: SQL string
Parser->>DB: execute generated SQL
DB-->>Parser: ingestion result
Parser->>Parser: validate schema against vinfo
Parser-->>Caller: ValidationResult
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
/hold |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/ingest_sqlite_test.go`:
- Around line 406-523: Add a focused negative test for
IngestSqliteWithCollection that passes a collectionID below zero and asserts the
call returns an error. Reuse the existing setup helpers in ingest_sqlite_test.go
(setupTestParser, createTestSQLite) and target the IngestSqliteWithCollection
method so the validation branch from ingest.go is covered; keep the assertion
specific to the invalid collectionID path and do not allow ingestion to proceed.
- Around line 450-452: The assertion in ingest_sqlite_test.go uses assert.Equal
with a literal message string, so the failure text won’t interpolate
collectionID and vmmoid. Update the VM ID check in the test to pass a formatted
message with the actual values (or otherwise build the message before calling
assert.Equal) so failures from the deterministic hash check clearly show the
collectionID and vmmoid used by the test.
In `@pkg/duckdb_parser/ingest.go`:
- Around line 30-35: The criticality check in the ingest flow is too broad
because it keys off `VMMOID` inside the statement, which wrongly relaxes SQLite
`INSERT INTO vinfo` failures. Update the logic around the ingestion path in
`duckdb_parser/ingest.go` so the relaxed handling is driven by the ingestion
source/policy (for example, the RVTools path) rather than SQL text contents.
Keep `IngestSqlite` behavior unchanged: check every error, wrap it with context,
and propagate it unless the source-specific policy explicitly marks it
non-critical.
In `@pkg/duckdb_parser/templates/ingest_sqlite.go.tmpl`:
- Line 79: The collection-mode filters in the ingest SQL template are too loose
because the current subqueries on vinfo can match any historical vmmoid and let
unrelated source VMs through. Update the related-table predicates in the
template so they are scoped to the current collection using the same EXISTS
pattern, and apply the change consistently in the vmemory, vdisk, and nic_flat
filter blocks as well as the vcpu case. Use the existing vinfo/vmmoid logic and
keep the collection_id check aligned with the surrounding ingest_sqlite.go.tmpl
conditions.
🪄 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: 5ac5b146-c8e6-42c2-a57f-55f9bf062a14
📒 Files selected for processing (4)
pkg/duckdb_parser/builder.gopkg/duckdb_parser/ingest.gopkg/duckdb_parser/ingest_sqlite_test.gopkg/duckdb_parser/templates/ingest_sqlite.go.tmpl
| func TestIngestSqliteWithCollection_HashesVMIDsAndSetsVmmoid(t *testing.T) { | ||
| const collectionID = int64(7) | ||
| ctx := context.Background() | ||
|
|
||
| parser, db, cleanup := setupTestParser(t, &testValidator{}) | ||
| defer cleanup() | ||
|
|
||
| addCollectionColumns(t, db) | ||
|
|
||
| clusters := []sqliteCluster{ | ||
| {id: "domain-c1", name: "cluster1", datacenter: "dc1"}, | ||
| } | ||
| vms := []sqliteVM{ | ||
| {id: "vm-001", name: "vm-1", clusterName: "cluster1"}, | ||
| {id: "vm-002", name: "vm-2", clusterName: "cluster1"}, | ||
| } | ||
| sqlitePath := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||
|
|
||
| result, err := parser.IngestSqliteWithCollection(ctx, sqlitePath, collectionID) | ||
| require.NoError(t, err) | ||
| require.True(t, result.IsValid()) | ||
|
|
||
| rows, err := db.QueryContext(ctx, `SELECT "VM ID", vmmoid, collection_id FROM vinfo`) | ||
| require.NoError(t, err) | ||
| defer func() { _ = rows.Close() }() | ||
|
|
||
| rowCount := 0 | ||
| for rows.Next() { | ||
| rowCount++ | ||
| var vmID, vmmoid string | ||
| var colID int64 | ||
| require.NoError(t, rows.Scan(&vmID, &vmmoid, &colID)) | ||
|
|
||
| // "VM ID" must be a 32-char lowercase hex md5 string. | ||
| assert.Len(t, vmID, 32, "VM ID should be a 32-char md5 hex string") | ||
| assert.Regexp(t, `^[0-9a-f]{32}$`, vmID, "VM ID should be a lowercase hex string") | ||
|
|
||
| // vmmoid is the original MOID (non-empty, not itself a 32-char hash). | ||
| assert.NotEmpty(t, vmmoid, "vmmoid should be the original MOID") | ||
| assert.NotEqual(t, 32, len(vmmoid), "vmmoid should not look like an md5 hash (original MOIDs are shorter)") | ||
|
|
||
| // collection_id must be set correctly. | ||
| assert.Equal(t, collectionID, colID, "collection_id should match the supplied collectionID") | ||
|
|
||
| // Hash must be deterministic: md5("{collectionID}_{vmmoid}") == vmID | ||
| expectedHash := fmt.Sprintf("%x", md5.Sum(fmt.Appendf(nil, "%d_%s", collectionID, vmmoid))) | ||
| assert.Equal(t, expectedHash, vmID, "VM ID should equal md5('%d_%s', collectionID, vmmoid)") | ||
| } | ||
| require.NoError(t, rows.Err()) | ||
| assert.Equal(t, len(vms), rowCount, "vinfo should have one row per non-template VM") | ||
| } | ||
|
|
||
| func TestIngestSqliteWithCollection_RelationalTablesMatchVinfoIDs(t *testing.T) { | ||
| const collectionID = int64(7) | ||
| ctx := context.Background() | ||
|
|
||
| parser, db, cleanup := setupTestParser(t, &testValidator{}) | ||
| defer cleanup() | ||
|
|
||
| addCollectionColumns(t, db) | ||
|
|
||
| clusters := []sqliteCluster{ | ||
| {id: "domain-c1", name: "cluster1", datacenter: "dc1"}, | ||
| } | ||
| vms := []sqliteVM{ | ||
| {id: "vm-001", name: "vm-1", clusterName: "cluster1"}, | ||
| {id: "vm-002", name: "vm-2", clusterName: "cluster1"}, | ||
| } | ||
| sqlitePath := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||
|
|
||
| _, err := parser.IngestSqliteWithCollection(ctx, sqlitePath, collectionID) | ||
| require.NoError(t, err) | ||
|
|
||
| // Every vcpu "VM ID" must exist in vinfo "VM ID" — no orphaned rows. | ||
| var orphans int | ||
| err = db.QueryRowContext(ctx, ` | ||
| SELECT COUNT(*) FROM vcpu | ||
| WHERE "VM ID" NOT IN (SELECT "VM ID" FROM vinfo) | ||
| `).Scan(&orphans) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 0, orphans, "all vcpu rows should reference a valid vinfo VM ID") | ||
| } | ||
|
|
||
| func TestIngestSqliteWithCollection_ZeroCollectionIDMatchesIngestSqlite(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| clusters := []sqliteCluster{ | ||
| {id: "domain-c1", name: "cluster1", datacenter: "dc1"}, | ||
| } | ||
| vms := []sqliteVM{ | ||
| {id: "vm-001", name: "vm-1", clusterName: "cluster1"}, | ||
| {id: "vm-002", name: "vm-2", clusterName: "cluster1"}, | ||
| } | ||
|
|
||
| // Parser A uses IngestSqlite (the zero-collection-ID path). | ||
| parserA, dbA, cleanupA := setupTestParser(t, &testValidator{}) | ||
| defer cleanupA() | ||
|
|
||
| // Parser B uses IngestSqliteWithCollection with collectionID=0. | ||
| parserB, dbB, cleanupB := setupTestParser(t, &testValidator{}) | ||
| defer cleanupB() | ||
|
|
||
| sqlitePathA := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||
| sqlitePathB := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||
|
|
||
| resultA, err := parserA.IngestSqlite(ctx, sqlitePathA) | ||
| require.NoError(t, err) | ||
|
|
||
| resultB, err := parserB.IngestSqliteWithCollection(ctx, sqlitePathB, 0) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, resultA.IsValid(), resultB.IsValid(), "validity should match between IngestSqlite and IngestSqliteWithCollection(0)") | ||
|
|
||
| var countA, countB int | ||
| require.NoError(t, dbA.QueryRowContext(ctx, `SELECT COUNT(*) FROM vinfo`).Scan(&countA)) | ||
| require.NoError(t, dbB.QueryRowContext(ctx, `SELECT COUNT(*) FROM vinfo`).Scan(&countB)) | ||
| assert.Equal(t, countA, countB, "VM count should match between IngestSqlite and IngestSqliteWithCollection(0)") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing test coverage for negative collectionID rejection.
IngestSqliteWithCollection rejects collectionID < 0 (see ingest.go's validation), but no test in this file exercises that path. Consider adding a case asserting the error for a negative collectionID to cover this critical validation branch.
As per path instructions, tests should "Strive for high test coverage on critical logic paths."
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 450-450: Detected use of the 'crypto/md5' package (md5.New / md5.Sum). MD5 is a cryptographically broken, collision-prone hash and must not be used for security purposes such as integrity checks, signatures, or password hashing. Use a strong hash from 'crypto/sha256' (sha256.New / sha256.Sum256) or 'crypto/sha512' instead; for passwords use a dedicated KDF such as golang.org/x/crypto/bcrypt or argon2.
Context: md5.Sum
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm.
(weak-hash-md5-go)
🪛 OpenGrep (1.23.0)
[WARNING] 451-451: MD5 and SHA1 are cryptographically broken and should not be used for security purposes. Use SHA-256 or stronger.
(coderabbit.crypto.go-weak-hash)
🤖 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/ingest_sqlite_test.go` around lines 406 - 523, Add a
focused negative test for IngestSqliteWithCollection that passes a collectionID
below zero and asserts the call returns an error. Reuse the existing setup
helpers in ingest_sqlite_test.go (setupTestParser, createTestSQLite) and target
the IngestSqliteWithCollection method so the validation branch from ingest.go is
covered; keep the assertion specific to the invalid collectionID path and do not
allow ingestion to proceed.
Source: Path instructions
| // Hash must be deterministic: md5("{collectionID}_{vmmoid}") == vmID | ||
| expectedHash := fmt.Sprintf("%x", md5.Sum(fmt.Appendf(nil, "%d_%s", collectionID, vmmoid))) | ||
| assert.Equal(t, expectedHash, vmID, "VM ID should equal md5('%d_%s', collectionID, vmmoid)") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assertion failure message won't interpolate values.
assert.Equal(t, expectedHash, vmID, "VM ID should equal md5('%d_%s', collectionID, vmmoid)") passes a single literal string as msgAndArgs; testify only applies fmt.Sprintf formatting when additional args follow the format string. On failure this message will print the literal text %d_%s instead of the actual collectionID/vmmoid values, making debugging harder.
🐛 Proposed fix
- assert.Equal(t, expectedHash, vmID, "VM ID should equal md5('%d_%s', collectionID, vmmoid)")
+ assert.Equal(t, expectedHash, vmID, "VM ID should equal md5(%d_%s)", collectionID, vmmoid)As per path instructions, "Assertions must be specific and provide informative messages on failure."
📝 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.
| // Hash must be deterministic: md5("{collectionID}_{vmmoid}") == vmID | |
| expectedHash := fmt.Sprintf("%x", md5.Sum(fmt.Appendf(nil, "%d_%s", collectionID, vmmoid))) | |
| assert.Equal(t, expectedHash, vmID, "VM ID should equal md5('%d_%s', collectionID, vmmoid)") | |
| // Hash must be deterministic: md5("{collectionID}_{vmmoid}") == vmID | |
| expectedHash := fmt.Sprintf("%x", md5.Sum(fmt.Appendf(nil, "%d_%s", collectionID, vmmoid))) | |
| assert.Equal(t, expectedHash, vmID, "VM ID should equal md5(%d_%s)", collectionID, vmmoid) |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 450-450: Detected use of the 'crypto/md5' package (md5.New / md5.Sum). MD5 is a cryptographically broken, collision-prone hash and must not be used for security purposes such as integrity checks, signatures, or password hashing. Use a strong hash from 'crypto/sha256' (sha256.New / sha256.Sum256) or 'crypto/sha512' instead; for passwords use a dedicated KDF such as golang.org/x/crypto/bcrypt or argon2.
Context: md5.Sum
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm.
(weak-hash-md5-go)
🪛 OpenGrep (1.23.0)
[WARNING] 451-451: MD5 and SHA1 are cryptographically broken and should not be used for security purposes. Use SHA-256 or stronger.
(coderabbit.crypto.go-weak-hash)
🤖 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/ingest_sqlite_test.go` around lines 450 - 452, The
assertion in ingest_sqlite_test.go uses assert.Equal with a literal message
string, so the failure text won’t interpolate collectionID and vmmoid. Update
the VM ID check in the test to pass a formatted message with the actual values
(or otherwise build the message before calling assert.Equal) so failures from
the deterministic hash check clearly show the collectionID and vmmoid used by
the test.
Source: Path instructions
| // INSERT INTO vinfo is critical only when it carries collection_id/vmmoid columns | ||
| // (SQLite collection path). The RVTools INSERT INTO vinfo does not include these columns | ||
| // and may legitimately fail when the source Excel is malformed. | ||
| if strings.Contains(upperStmt, "INSERT INTO VINFO") && strings.Contains(upperStmt, "VMMOID") { | ||
| return true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not use VMMOID as the proxy for RVTools-only leniency.
This also makes the SQLite collectionID=0 INSERT INTO vinfo non-critical, because that template path has no vmmoid column. A failed SQLite vinfo load can now continue without an ingestion error, which is not the same as preserving existing IngestSqlite behavior. Scope the relaxed behavior by ingestion source/policy instead of statement contents. As per path instructions, 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/ingest.go` around lines 30 - 35, The criticality check in
the ingest flow is too broad because it keys off `VMMOID` inside the statement,
which wrongly relaxes SQLite `INSERT INTO vinfo` failures. Update the logic
around the ingestion path in `duckdb_parser/ingest.go` so the relaxed handling
is driven by the ingestion source/policy (for example, the RVTools path) rather
than SQL text contents. Keep `IngestSqlite` behavior unchanged: check every
error, wrap it with context, and propagate it unless the source-specific policy
explicitly marks it non-critical.
Source: Path instructions
| v.CoresPerSocket | ||
| FROM src.VM v | ||
| WHERE v.ID IN (SELECT "VM ID" FROM vinfo); | ||
| WHERE v.ID IN (SELECT {{if .CollectionID}}vmmoid{{else}}"VM ID"{{end}} FROM vinfo{{if .CollectionID}} WHERE vmmoid IS NOT NULL{{end}}); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope related-table filters to the current collection.
Under collection mode these subqueries match any historical vinfo.vmmoid. If vinfo already contains another collection, a current source VM can pass this filter without a matching vinfo row for the current collection_id, producing orphaned hashed rows in vcpu, vmemory, vdisk, or vnetwork.
🔧 Proposed fix
-WHERE v.ID IN (SELECT {{if .CollectionID}}vmmoid{{else}}"VM ID"{{end}} FROM vinfo{{if .CollectionID}} WHERE vmmoid IS NOT NULL{{end}});
+WHERE {{if .CollectionID}}EXISTS (
+ SELECT 1 FROM vinfo
+ WHERE vinfo.vmmoid = v.ID
+ AND vinfo.collection_id = {{.CollectionID}}
+){{else}}v.ID IN (SELECT "VM ID" FROM vinfo){{end}};Apply the same EXISTS shape to the vmemory, vdisk, and nic_flat filters.
Also applies to: 87-87, 112-112, 125-125
🤖 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_sqlite.go.tmpl` at line 79, The
collection-mode filters in the ingest SQL template are too loose because the
current subqueries on vinfo can match any historical vmmoid and let unrelated
source VMs through. Update the related-table predicates in the template so they
are scoped to the current collection using the same EXISTS pattern, and apply
the change consistently in the vmemory, vdisk, and nic_flat filter blocks as
well as the vcpu case. Use the existing vinfo/vmmoid logic and keep the
collection_id check aligned with the surrounding ingest_sqlite.go.tmpl
conditions.
|
@amalimov I add the |
Added IngestSqliteWithCollection:
This will add a new column "collection_id" (representing one collection of data from vsphere).
also, when collectionID > 0, the "VM ID" column will get as value the hash of <collectionID>_<MOID> and the original MOID will be stored in a new field: "vmmoid".
This should be backwards compatible, unobstructive for the rvtools ingestion path, and serve the new collection dimension ("scheduled updates") in the agent's data layer.
Signed-off-by: Ami Malimovka <amalimov@redhat.com>
c4925b7 to
aa13fa6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ingest_sqlite_test.go`:
- Around line 458-486: The collection-aware relationship test in
TestIngestSqliteWithCollection_RelationalTablesMatchVinfoIDs only validates
vcpu, so it misses the other affected tables. Extend the fixture data in
createTestSQLite for this test to include disk and NIC records, then add orphan
checks against vinfo for vmemory, vdisk, and vnetwork in the same test so every
collection-aware relationship is covered.
- Around line 517-522: The existing comparison in the IngestSqlite test only
checks validity and row counts, which can miss regressions where IDs change but
totals stay the same. Update the assertions in the test around resultA/resultB
and the vinfo query to compare ordered representative "VM ID" values from both
parsers, and also verify at least one related-table "VM ID" matches between
them; use specific, informative assertion messages that clearly identify which
ID set differed.
🪄 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: 9658627c-1ad9-42d7-a70e-1be98fb66f22
📒 Files selected for processing (4)
pkg/duckdb_parser/builder.gopkg/duckdb_parser/ingest.gopkg/duckdb_parser/ingest_sqlite_test.gopkg/duckdb_parser/templates/ingest_sqlite.go.tmpl
| func TestIngestSqliteWithCollection_RelationalTablesMatchVinfoIDs(t *testing.T) { | ||
| const collectionID = int64(7) | ||
| ctx := context.Background() | ||
|
|
||
| parser, db, cleanup := setupTestParser(t, &testValidator{}) | ||
| defer cleanup() | ||
|
|
||
| addCollectionColumns(t, db) | ||
|
|
||
| clusters := []sqliteCluster{ | ||
| {id: "domain-c1", name: "cluster1", datacenter: "dc1"}, | ||
| } | ||
| vms := []sqliteVM{ | ||
| {id: "vm-001", name: "vm-1", clusterName: "cluster1"}, | ||
| {id: "vm-002", name: "vm-2", clusterName: "cluster1"}, | ||
| } | ||
| sqlitePath := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||
|
|
||
| _, err := parser.IngestSqliteWithCollection(ctx, sqlitePath, collectionID) | ||
| require.NoError(t, err) | ||
|
|
||
| // Every vcpu "VM ID" must exist in vinfo "VM ID" — no orphaned rows. | ||
| var orphans int | ||
| err = db.QueryRowContext(ctx, ` | ||
| SELECT COUNT(*) FROM vcpu | ||
| WHERE "VM ID" NOT IN (SELECT "VM ID" FROM vinfo) | ||
| `).Scan(&orphans) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 0, orphans, "all vcpu rows should reference a valid vinfo VM ID") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Exercise every collection-aware relationship, not only vcpu.
The template changed vcpu, vmemory, vdisk, and vnetwork, but this test only checks vcpu, and the fixture has empty disks/NICs. Add disk/NIC fixture data and assert no orphans for vmemory, vdisk, and vnetwork too. As per path instructions, tests should “Strive for high test coverage on critical logic paths.”
🤖 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/ingest_sqlite_test.go` around lines 458 - 486, The
collection-aware relationship test in
TestIngestSqliteWithCollection_RelationalTablesMatchVinfoIDs only validates
vcpu, so it misses the other affected tables. Extend the fixture data in
createTestSQLite for this test to include disk and NIC records, then add orphan
checks against vinfo for vmemory, vdisk, and vnetwork in the same test so every
collection-aware relationship is covered.
Source: Path instructions
| assert.Equal(t, resultA.IsValid(), resultB.IsValid(), "validity should match between IngestSqlite and IngestSqliteWithCollection(0)") | ||
|
|
||
| var countA, countB int | ||
| require.NoError(t, dbA.QueryRowContext(ctx, `SELECT COUNT(*) FROM vinfo`).Scan(&countA)) | ||
| require.NoError(t, dbB.QueryRowContext(ctx, `SELECT COUNT(*) FROM vinfo`).Scan(&countB)) | ||
| assert.Equal(t, countA, countB, "VM count should match between IngestSqlite and IngestSqliteWithCollection(0)") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Compare representative IDs, not just row counts.
Backward compatibility for collectionID=0 would still pass here if VM IDs or related rows changed but counts stayed the same. Compare ordered "VM ID" values, and at least one related-table "VM ID", between both parsers. As per path instructions, “Assertions must be specific and provide informative messages on failure.”
🤖 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/ingest_sqlite_test.go` around lines 517 - 522, The existing
comparison in the IngestSqlite test only checks validity and row counts, which
can miss regressions where IDs change but totals stay the same. Update the
assertions in the test around resultA/resultB and the vinfo query to compare
ordered representative "VM ID" values from both parsers, and also verify at
least one related-table "VM ID" matches between them; use specific, informative
assertion messages that clearly identify which ID set differed.
Source: Path instructions
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
Added IngestSqliteWithCollection:
This will add a new column "collection_id" (representing one collection of data from vsphere). also, when collectionID > 0, the "VM ID" column will get as value the hash of _ and the original MOID will be stored in a new field: "vmmoid".
This should be backwards compatible, unobstructive for the rvtools ingestion path, and serve the new collection dimension ("scheduled updates") in the agent's data layer.
Summary by CodeRabbit
New Features
Bug Fixes