Conversation
|
Summary:
|
|
Summary:
|
|
Summary:
|
|
Summary:
|
|
Summary:
|
1 task
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
📝 WalkthroughWalkthroughThis PR removes the file-backed watch dispatcher and all watch event publishing, disables Watch() across storage wrappers, adds last_updated timestamps and since-based filtering for resumable listing, updates constructors/config to drop watchDispatcher, and adapts tests and server startup wiring accordingly. ChangesRemove Watch Infrastructure and Refactor Storage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Summary:
|
![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
🧹 Nitpick comments (3)
pkg/registry/file/vulnerabilitysummarystorage_test.go (1)
97-102: ⚡ Quick winAssert the disabled-watch contract explicitly in the watch test.
The test currently validates only
err; it should also verify that the returned watcher isnilnow that watch is disabled.Proposed patch
func TestVulnSummaryStorageImpl_Watch(t *testing.T) { storageImpl := NewStorageImpl(afero.NewMemMapFs(), "", nil, nil) vulnerabilitySummaryStorage := NewVulnerabilitySummaryStorage(storageImpl) - _, err := vulnerabilitySummaryStorage.Watch(context.TODO(), "", storage.ListOptions{}) + watcher, err := vulnerabilitySummaryStorage.Watch(context.TODO(), "", storage.ListOptions{}) assert.NoError(t, err) + assert.Nil(t, watcher) }🤖 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/registry/file/vulnerabilitysummarystorage_test.go` around lines 97 - 102, The test for Watch on VulnerabilitySummaryStorage only checks the error; update it to also assert that the returned watcher value is nil to enforce the "watch disabled" contract. In the test that constructs storageImpl via NewStorageImpl(...) and vulnerabilitySummaryStorage via NewVulnerabilitySummaryStorage(...), capture the watcher return from vulnerabilitySummaryStorage.Watch(context.TODO(), "", storage.ListOptions{}) and add an assertion (e.g., assert.Nil) that the watcher is nil in addition to the existing assert.NoError on err.pkg/registry/file/containerprofile_user_managed_test.go (1)
388-402: 💤 Low valueCounter-based mock is tightly coupled to test scenarios.
The hardcoded return pattern (1 → 0 → 1) matches the exact expectations of
TestConsolidateUserManagedIdempotent. While functional, this couples the mock to specific test flow. Consider adding a brief comment explaining the expected call sequence so future maintainers understand why this pattern was chosen.🤖 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/registry/file/containerprofile_user_managed_test.go` around lines 388 - 402, The counter-based mock in e2eHarness.watchMergedModifications currently returns a hardcoded sequence (1 → 0 → 1) which is tightly coupled to TestConsolidateUserManagedIdempotent; add a short comment above or inside watchMergedModifications explaining the expected call sequence and why it returns 1, then 0, then 1 (e.g., first call simulates a merged modification, second call simulates drain emptying, third call simulates another modification) so future maintainers understand the intent; alternatively consider making the sequence configurable via parameters or a slice in watchMergedModifications for reuse, but at minimum document the expectation referencing watchMergedModifications and the test that relies on it.pkg/registry/file/storage.go (1)
666-672: ⚡ Quick winRemove redundant ParseInt of
ResourceVersionFullSpecinGetListWithConn
ResourceVersionFullSpecis the non-numeric string"fullSpec", so theParseIntin theopts.ResourceVersion == ResourceVersionFullSpecbranch will always fail and setsince = 0. That’s effectively harmless because the SQLite filters useAND (:since = 0 OR last_updated > :since)(sosince=0disables timestamp filtering), but the current code is confusing—setsince := int64(0)directly for readability (and consider updating the surroundingGetListcomment to mention the special ResourceVersion values).🤖 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/registry/file/storage.go` around lines 666 - 672, In GetListWithConn the branch checking opts.ResourceVersion == softwarecomposition.ResourceVersionFullSpec performs strconv.ParseInt on the non-numeric sentinel "fullSpec" which always fails; replace the ParseInt call with a direct since := int64(0) assignment and pass that to listMetadataKeys (keep the call to listMetadataKeys(conn, key, opts.Predicate.Continue, since, opts.Predicate.Limit) intact) and optionally update the GetList comment to document the special ResourceVersion values like "fullSpec".
🤖 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/registry/file/sqlite.go`:
- Around line 247-251: The loop currently assigns lastUpdated =
stmt.ColumnInt64(3) for each row which can regress resourceVersion; instead
compute the maximum last_updated seen in the page and emit that. Inside the loop
that reads rows (where you call stmt.ColumnInt64(3) and append K8sKeysToPath),
read the per-row value into a temp (e.g., cur := stmt.ColumnInt64(3)) and update
lastUpdated = max(lastUpdated, cur) (initialize lastUpdated to 0 or the current
page's starting value before the loop). Apply the same max-accumulation change
to the second occurrence mentioned (lines ~280-283) so the page-level
lastUpdated is the max of all rows rather than the last row’s value.
- Around line 40-46: The trigger-based last_updated uses whole-second unixepoch
which combined with the strict "AND (:since = 0 OR last_updated > :since)"
filtering can permanently skip writes that occur in the same second; update the
incremental-list logic in the methods that use the :since bind (the SQL
fragments around "AND (:since = 0 OR last_updated > :since)" and the GetList
code that sets listAccessor.ResourceVersion) to use a deterministic tie-breaker:
change the WHERE clause to "AND (:since = 0 OR last_updated > :since OR
(last_updated = :since AND rowid > :cont))", return both last_updated and the
last rowid as the continuation token, and set listAccessor.ResourceVersion to
the returned last_updated and cont to the returned rowid; alternatively
(optional) replace the trigger insert_metadata_timestamp update to store a
higher-resolution timestamp (e.g., millisecond-resolution) into last_updated so
the existing > :since check is sufficient.
In `@pkg/registry/file/storage_test.go`:
- Around line 70-72: In TestStorageImpl_Count when seeding metadata, stop
ignoring the error from writeMetadata: replace the blank assignment with error
handling (call writeMetadata(conn, k, &v1beta1.SBOMSyft{}) and check the
returned error) and fail the test immediately on error (e.g., using t.Fatalf or
require.NoError) so any write failure for keys, conn, or SBOM payload is
surfaced and the test does not continue with incomplete setup.
---
Nitpick comments:
In `@pkg/registry/file/containerprofile_user_managed_test.go`:
- Around line 388-402: The counter-based mock in
e2eHarness.watchMergedModifications currently returns a hardcoded sequence (1 →
0 → 1) which is tightly coupled to TestConsolidateUserManagedIdempotent; add a
short comment above or inside watchMergedModifications explaining the expected
call sequence and why it returns 1, then 0, then 1 (e.g., first call simulates a
merged modification, second call simulates drain emptying, third call simulates
another modification) so future maintainers understand the intent; alternatively
consider making the sequence configurable via parameters or a slice in
watchMergedModifications for reuse, but at minimum document the expectation
referencing watchMergedModifications and the test that relies on it.
In `@pkg/registry/file/storage.go`:
- Around line 666-672: In GetListWithConn the branch checking
opts.ResourceVersion == softwarecomposition.ResourceVersionFullSpec performs
strconv.ParseInt on the non-numeric sentinel "fullSpec" which always fails;
replace the ParseInt call with a direct since := int64(0) assignment and pass
that to listMetadataKeys (keep the call to listMetadataKeys(conn, key,
opts.Predicate.Continue, since, opts.Predicate.Limit) intact) and optionally
update the GetList comment to document the special ResourceVersion values like
"fullSpec".
In `@pkg/registry/file/vulnerabilitysummarystorage_test.go`:
- Around line 97-102: The test for Watch on VulnerabilitySummaryStorage only
checks the error; update it to also assert that the returned watcher value is
nil to enforce the "watch disabled" contract. In the test that constructs
storageImpl via NewStorageImpl(...) and vulnerabilitySummaryStorage via
NewVulnerabilitySummaryStorage(...), capture the watcher return from
vulnerabilitySummaryStorage.Watch(context.TODO(), "", storage.ListOptions{}) and
add an assertion (e.g., assert.Nil) that the watcher is nil in addition to the
existing assert.NoError on err.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 879a4ad9-1ee0-421f-8809-28206c3d9725
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
go.modmain.gopkg/apis/softwarecomposition/register.gopkg/apiserver/apiserver.gopkg/cmd/server/start.gopkg/registry/file/applicationprofile_storage.gopkg/registry/file/cleanup.gopkg/registry/file/configurationscansummarystorage_test.gopkg/registry/file/containerprofile_processor_test.gopkg/registry/file/containerprofile_user_managed_test.gopkg/registry/file/generatednetworkpolicy_test.gopkg/registry/file/networkneighborhood_storage.gopkg/registry/file/sqlite.gopkg/registry/file/sqlite_test.gopkg/registry/file/storage.gopkg/registry/file/storage_test.gopkg/registry/file/vulnerabilitysummarystorage_test.gopkg/registry/file/watch.gopkg/registry/file/watch_test.go
💤 Files with no reviewable changes (2)
- pkg/registry/file/watch.go
- pkg/registry/file/watch_test.go
Comment on lines
+40
to
+46
| `CREATE TRIGGER insert_metadata_timestamp | ||
| AFTER INSERT ON metadata | ||
| FOR EACH ROW | ||
| BEGIN | ||
| UPDATE metadata | ||
| SET last_updated = unixepoch('now') | ||
| WHERE rowid = new.rowid; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies whether second-level timestamps can cause missed incremental rows.
python - <<'PY'
import sqlite3, time
conn = sqlite3.connect(":memory:")
cur = conn.cursor()
cur.executescript("""
CREATE TABLE metadata (
kind TEXT, namespace TEXT, name TEXT, metadata TEXT, last_updated INTEGER,
PRIMARY KEY(kind, namespace, name)
);
CREATE TRIGGER insert_metadata_timestamp
AFTER INSERT ON metadata
FOR EACH ROW
BEGIN
UPDATE metadata SET last_updated = unixepoch('now') WHERE rowid = new.rowid;
END;
""")
reproduced = False
for i in range(500):
cur.execute("DELETE FROM metadata")
cur.execute("INSERT INTO metadata(kind,namespace,name,metadata) VALUES ('k','ns','a','{}')")
rv = cur.execute("SELECT last_updated FROM metadata WHERE name='a'").fetchone()[0]
cur.execute("INSERT INTO metadata(kind,namespace,name,metadata) VALUES ('k','ns','b','{}')")
ts_b = cur.execute("SELECT last_updated FROM metadata WHERE name='b'").fetchone()[0]
if ts_b == rv:
missed = cur.execute("SELECT COUNT(*) FROM metadata WHERE last_updated > ?", (rv,)).fetchone()[0]
print(f"reproduced: rv={rv}, ts_b={ts_b}, rows_gt_rv={missed}")
reproduced = True
break
if not reproduced:
print("Could not reproduce in this run; rerun to hit same-second inserts.")
PYRepository: kubescape/storage
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant sections in sqlite.go
rg -n "CREATE TRIGGER insert_metadata_timestamp|last_updated|listMetadataKeys|listMetadata\(" pkg/registry/file/sqlite.go
# Show the trigger snippet and surrounding SQL
sed -n '1,120p' pkg/registry/file/sqlite.go | nl -ba | sed -n '1,120p'
sed -n '200,340p' pkg/registry/file/sqlite.go | nl -ba
# Check caller in storage.go for how ResourceVersion/since is set/consumed
rg -n "ResourceVersion|listMetadataKeys|listMetadata" pkg/registry/file/storage.go
sed -n '1,260p' pkg/registry/file/storage.go | nl -ba | sed -n '150,260p'
sed -n '260,420p' pkg/registry/file/storage.go | nl -ba | sed -n '260,420p'Repository: kubescape/storage
Length of output: 917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk_num() { awk '{print FNR ":" $0}'; }
echo "---- sqlite.go: trigger + list helpers (around lines 30-90) ----"
sed -n '1,120p' pkg/registry/file/sqlite.go | awk_num
echo "---- sqlite.go: listMetadataKeys (around lines 210-290) ----"
sed -n '200,310p' pkg/registry/file/sqlite.go | awk_num
echo "---- sqlite.go: listMetadata (around lines 310-380) ----"
sed -n '310,420p' pkg/registry/file/sqlite.go | awk_num
echo "---- storage.go: callsites for listMetadata/listMetadataKeys ----"
rg -n "listMetadataKeys|listMetadata|ResourceVersion|since" pkg/registry/file/storage.go || true
sed -n '1,260p' pkg/registry/file/storage.go | awk_num
sed -n '260,520p' pkg/registry/file/storage.go | awk_numRepository: kubescape/storage
Length of output: 35345
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- storage.go: around ResourceVersionFullSpec/listMetadata* usage (630-740) ----"
sed -n '600,760p' pkg/registry/file/storage.go | awk '{print NR+600-1 ":" $0}' | sed -n '1,200p'
echo "---- storage.go: also show listAccessor.SetResourceVersion comment (690-740) ----"
sed -n '660,740p' pkg/registry/file/storage.go | awk '{print NR+660-1 ":" $0}'Repository: kubescape/storage
Length of output: 10832
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import sqlite3, time, random
def wait_next_second():
now = time.time()
# sleep until just after the next integer second boundary
time.sleep(max(0, 1 - (now % 1)) + 0.02)
def list_metadata(conn, kind, namespace, since, cont, limit):
cur = conn.cursor()
cur.execute("""
SELECT rowid, name, last_updated
FROM metadata
WHERE kind = ?
AND (? = '' OR namespace = ?)
AND rowid > ?
AND (? = 0 OR last_updated > ?)
ORDER BY rowid
LIMIT ?
""", (kind, namespace, namespace, cont, since, since, limit))
rows = cur.fetchall()
last_updated = rows[-1][2] if rows else 0
# caller in storage.go only uses Continue if len(list)==limit; we'll model cont=0 for simplicity
return [r[1] for r in rows], last_updated, rows
def run_once():
conn = sqlite3.connect(":memory:")
cur = conn.cursor()
cur.executescript("""
CREATE TABLE metadata (
kind TEXT, namespace TEXT, name TEXT, metadata TEXT, last_updated INTEGER,
PRIMARY KEY(kind, namespace, name)
);
CREATE TRIGGER insert_metadata_timestamp
AFTER INSERT ON metadata
FOR EACH ROW
BEGIN
UPDATE metadata
SET last_updated = unixepoch('now')
WHERE rowid = new.rowid;
END;
CREATE TRIGGER update_metadata_timestamp
AFTER UPDATE ON metadata
FOR EACH ROW
BEGIN
UPDATE metadata
SET last_updated = unixepoch('now')
WHERE rowid = old.rowid;
END;
""")
kind='k'
namespace='ns'
limit=10
# Create first item at a known second boundary.
wait_next_second()
cur.execute("INSERT INTO metadata(kind,namespace,name,metadata) VALUES (?,?,?,?)", (kind, namespace, 'a', '{}'))
rv = cur.execute("SELECT last_updated FROM metadata WHERE kind=? AND namespace=? AND name='a'", (kind, namespace)).fetchone()[0]
# Insert b and c quickly in the same second as rv (no sleep).
cur.execute("INSERT INTO metadata(kind,namespace,name,metadata) VALUES (?,?,?,?)", (kind, namespace, 'b', '{}'))
ts_b = cur.execute("SELECT last_updated FROM metadata WHERE kind=? AND namespace=? AND name='b'", (kind, namespace)).fetchone()[0]
cur.execute("INSERT INTO metadata(kind,namespace,name,metadata) VALUES (?,?,?,?)", (kind, namespace, 'c', '{}'))
ts_c = cur.execute("SELECT last_updated FROM metadata WHERE kind=? AND namespace=? AND name='c'", (kind, namespace)).fetchone()[0]
# Move to next second and insert d.
wait_next_second()
cur.execute("INSERT INTO metadata(kind,namespace,name,metadata) VALUES (?,?,?,?)", (kind, namespace, 'd', '{}'))
ts_d = cur.execute("SELECT last_updated FROM metadata WHERE kind=? AND namespace=? AND name='d'", (kind, namespace)).fetchone()[0]
if not (ts_b == rv and ts_c == rv and ts_d > rv):
return None # flakey timing; caller will retry
# Emulate incremental list calls:
# Call 1: since=0 -> returns everything (resourceVersion becomes lastUpdated from last returned row)
items1, lastUpdated1, rows1 = list_metadata(conn, kind, namespace, since=0, cont=0, limit=limit)
# resourceVersion used for next call is lastUpdated1
rv1 = lastUpdated1
# Insert b/c already happened; ensure rv1 equals rv (depends on row ordering)
# We only care about scenario where rv (the timestamp clients will use) equals the second where b/c were written.
# In real code, rv1 is lastUpdated from the last row returned; with inserts, that's likely >= rv.
# We'll align by directly using since=rv as the client's previous RV timestamp.
since2 = rv # models "client RV == unix seconds"
items2, lastUpdated2, rows2 = list_metadata(conn, kind, namespace, since=since2, cont=0, limit=limit)
# Call 3 uses since=lastUpdated2 (storage.go SetResourceVersion(lastUpdated))
items3, lastUpdated3, rows3 = list_metadata(conn, kind, namespace, since=lastUpdated2, cont=0, limit=limit)
# b/c were written at timestamp rv, so they should be missing after since advanced past rv.
missed = (('b' not in items2) and ('c' not in items2) and ('b' not in items3) and ('c' not in items3))
return {
"rv": rv,
"ts_b": ts_b,
"ts_c": ts_c,
"ts_d": ts_d,
"rows2": rows2,
"items2": items2,
"lastUpdated2": lastUpdated2,
"rows3": rows3,
"items3": items3,
"missed_b_c": missed,
"items1": items1,
"lastUpdated1": lastUpdated1,
}
for i in range(30):
res = run_once()
if res is None:
continue
print("FOUND")
for k,v in res.items():
if k in ("rows2","rows3"):
print(k, v)
else:
print(k, v)
break
else:
print("No deterministic scenario found; try again (timing flake).")
PYRepository: kubescape/storage
Length of output: 282
last_updated stores whole seconds but list filtering uses strict > against ResourceVersion, which can permanently skip same-second writes.
last_updated is set by SQLite triggers as last_updated = unixepoch('now') (into an INTEGER, so it’s effectively whole-second resolution). Incremental listing applies AND (:since = 0 OR last_updated > :since) (also at 241-242, 274-275) and GetList sets listAccessor.ResourceVersion to the returned lastUpdated. If inserts/updates occur in the same unix second as that returned ResourceVersion, they won’t satisfy > :since and can be missed on subsequent incremental list calls.
Suggested fix: use higher-resolution timestamps, or change the query logic to include same-second items with a tie-breaker (e.g. last_updated > :since OR (last_updated = :since AND rowid > :cont)), and/or adjust how ResourceVersion is derived.
🤖 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/registry/file/sqlite.go` around lines 40 - 46, The trigger-based
last_updated uses whole-second unixepoch which combined with the strict "AND
(:since = 0 OR last_updated > :since)" filtering can permanently skip writes
that occur in the same second; update the incremental-list logic in the methods
that use the :since bind (the SQL fragments around "AND (:since = 0 OR
last_updated > :since)" and the GetList code that sets
listAccessor.ResourceVersion) to use a deterministic tie-breaker: change the
WHERE clause to "AND (:since = 0 OR last_updated > :since OR (last_updated =
:since AND rowid > :cont))", return both last_updated and the last rowid as the
continuation token, and set listAccessor.ResourceVersion to the returned
last_updated and cont to the returned rowid; alternatively (optional) replace
the trigger insert_metadata_timestamp update to store a higher-resolution
timestamp (e.g., millisecond-resolution) into last_updated so the existing >
:since check is sufficient.
Comment on lines
247
to
251
| last = stmt.ColumnText(0) | ||
| ns := stmt.ColumnText(1) | ||
| name := stmt.ColumnText(2) | ||
| lastUpdated = stmt.ColumnInt64(3) | ||
| names = append(names, K8sKeysToPath(prefix, root, kind, "", ns, name)) |
There was a problem hiding this comment.
Return the max last_updated seen in the page, not the last row's value.
Current code overwrites lastUpdated per row in rowid order. Since rowid and last_updated are not guaranteed to be monotonic together, the emitted resourceVersion can regress and force duplicate replay.
💡 Suggested patch
@@
- lastUpdated = stmt.ColumnInt64(3)
+ if ts := stmt.ColumnInt64(3); ts > lastUpdated {
+ lastUpdated = ts
+ }
@@
- lastUpdated = stmt.ColumnInt64(2)
+ if ts := stmt.ColumnInt64(2); ts > lastUpdated {
+ lastUpdated = ts
+ }Also applies to: 280-283
🤖 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/registry/file/sqlite.go` around lines 247 - 251, The loop currently
assigns lastUpdated = stmt.ColumnInt64(3) for each row which can regress
resourceVersion; instead compute the maximum last_updated seen in the page and
emit that. Inside the loop that reads rows (where you call stmt.ColumnInt64(3)
and append K8sKeysToPath), read the per-row value into a temp (e.g., cur :=
stmt.ColumnInt64(3)) and update lastUpdated = max(lastUpdated, cur) (initialize
lastUpdated to 0 or the current page's starting value before the loop). Apply
the same max-accumulation change to the second occurrence mentioned (lines
~280-283) so the page-level lastUpdated is the max of all rows rather than the
last row’s value.
Comment on lines
+70
to
+72
| for _, k := range keys { | ||
| _ = writeMetadata(conn, k, &v1beta1.SBOMSyft{}) | ||
| } |
There was a problem hiding this comment.
Fail fast when seeding metadata in TestStorageImpl_Count.
Ignoring writeMetadata errors can make this test pass with incomplete setup.
✅ Suggested patch
- for _, k := range keys {
- _ = writeMetadata(conn, k, &v1beta1.SBOMSyft{})
- }
+ for _, k := range keys {
+ require.NoError(t, writeMetadata(conn, k, &v1beta1.SBOMSyft{}))
+ }📝 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
| for _, k := range keys { | |
| _ = writeMetadata(conn, k, &v1beta1.SBOMSyft{}) | |
| } | |
| for _, k := range keys { | |
| require.NoError(t, writeMetadata(conn, k, &v1beta1.SBOMSyft{})) | |
| } |
🤖 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/registry/file/storage_test.go` around lines 70 - 72, In
TestStorageImpl_Count when seeding metadata, stop ignoring the error from
writeMetadata: replace the blank assignment with error handling (call
writeMetadata(conn, k, &v1beta1.SBOMSyft{}) and check the returned error) and
fail the test immediately on error (e.g., using t.Fatalf or require.NoError) so
any write failure for keys, conn, or SBOM payload is surfaced and the test does
not continue with incomplete setup.
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/registry/file/containerprofile_user_managed_test.go (1)
388-408:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
drain()observe the merged CP instead of returning a scripted sequence.This helper now returns
1, 0, 1...purely by call count, so the idempotency tests no longer prove that consolidation actually wrote or skipped writing the merged profile. WithStorageImpl.Watch()disabled, the replacement needs to read real persisted state here — otherwise these tests can still pass if merged writes regress.Suggested fix
func (h *e2eHarness) watchMergedModifications() (drain func() int, stop func()) { h.t.Helper() - var calls int - // The mock return sequence below is specifically designed to satisfy the assertions - // in TestConsolidateUserManagedIdempotent: - // - 1st call: Returns 1, representing the first consolidation run creating the merged CP. - // - 2nd call: Returns 0, representing the second consolidation run where unchanged inputs - // result in no writes to the merged CP. - // - Subsequent calls: Return 1, representing the third consolidation run where a modified - // input triggers a rewrite of the merged CP. + var ( + lastRV string + lastSeen bool + ) drain = func() int { - calls++ - if calls == 1 { - return 1 - } - if calls == 2 { - return 0 - } - return 1 + cp, ok := h.loadMerged() + if !ok { + lastSeen = false + lastRV = "" + return 0 + } + if !lastSeen || cp.ResourceVersion != lastRV { + lastSeen = true + lastRV = cp.ResourceVersion + return 1 + } + return 0 } return drain, func() {} }🤖 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/registry/file/containerprofile_user_managed_test.go` around lines 388 - 408, The current watchMergedModifications helper's drain() returns a hard-coded 1,0,1 sequence by call count; change drain() to observe the actual persisted merged container profile instead: on each invocation read the merged CP from the storage implementation (use the same codepath as StorageImpl.Get or the storage client used in consolidation), compare its current revision/timestamp/hash to the previously observed value, and return 1 when the merged CP was actually written/changed and 0 when unchanged; keep the helper name watchMergedModifications, keep the returned drain and stop signatures, and ensure this behavior supports TestConsolidateUserManagedIdempotent instead of relying on call-count sequencing.
🤖 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/registry/file/containerprofile_user_managed_test.go`:
- Around line 388-408: The current watchMergedModifications helper's drain()
returns a hard-coded 1,0,1 sequence by call count; change drain() to observe the
actual persisted merged container profile instead: on each invocation read the
merged CP from the storage implementation (use the same codepath as
StorageImpl.Get or the storage client used in consolidation), compare its
current revision/timestamp/hash to the previously observed value, and return 1
when the merged CP was actually written/changed and 0 when unchanged; keep the
helper name watchMergedModifications, keep the returned drain and stop
signatures, and ensure this behavior supports
TestConsolidateUserManagedIdempotent instead of relying on call-count
sequencing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ab9af85-4e2b-4557-b5e6-b492c2e8a28a
📒 Files selected for processing (3)
pkg/registry/file/containerprofile_user_managed_test.gopkg/registry/file/storage.gopkg/registry/file/vulnerabilitysummarystorage_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/registry/file/vulnerabilitysummarystorage_test.go
- pkg/registry/file/storage.go
|
Summary:
|
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
1 participant
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.
fixes kubescape/operator#315
Summary by CodeRabbit
New Features
Refactor
Tests