UPSTREAM: <carry>: Add ListPackageCustomSchemas gRPC endpoint to opm serve#1296
Draft
perdasilva wants to merge 1 commit into
Draft
UPSTREAM: <carry>: Add ListPackageCustomSchemas gRPC endpoint to opm serve#1296perdasilva wants to merge 1 commit into
perdasilva wants to merge 1 commit into
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva 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 |
openshift-ci
Bot
added
the
approved
WalkthroughAdds a server-streaming gRPC RPC ChangesPackage-scoped custom-schema streaming
sequenceDiagram
participant Client
participant Server as gRPC Server
participant Cache as Cache Layer
participant Backend as Storage Backend
Client->>Server: ListPackageCustomSchemas(schema, packageName)
Server->>Server: validate request fields
Server->>Cache: ListPackageCustomSchemas(ctx, schema, packageName, sender)
Cache->>Backend: SendMetas(metaKey, sender)
activate Backend
loop for each stored meta blob
Backend->>Backend: read blob bytes
Backend->>Cache: sender(blob)
end
deactivate Backend
Cache->>Server: return (possibly error)
Server->>Server: unmarshal JSON -> map
Server->>Server: convert map -> google.protobuf.Struct
Server-->>Client: stream Struct messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🧹 Nitpick comments (1)
staging/operator-registry/pkg/server/server_test.go (1)
812-815: ⚡ Quick winCover the unfiltered RPC path against the populated custom-schema cache.
Right now the empty-request case only runs against
cacheAddress, so it would still pass if the handler accidentally treated an emptySchemaas “return nothing.” Adding an empty-schema case againstcustomSchemaCacheAddressand asserting the two distinct resource names would make this new RPC coverage much harder to fool.Also applies to: 827-841, 850-862
🤖 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 `@staging/operator-registry/pkg/server/server_test.go` around lines 812 - 815, Add an empty-request test run that targets the populated custom-schema cache: in TestGetCustomSchemas call testGetCustomSchemasEmpty(customSchemaCacheAddress) (in addition to the existing cacheAddress case) and assert the response returns the two distinct resource names expected; likewise update the related test runs referenced (the blocks around lines 827–841 and 850–862) to invoke the empty-schema path against customSchemaCacheAddress as well so the unfiltered RPC path is exercised and the two resource names are asserted.
🤖 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 `@staging/operator-registry/pkg/cache/json.go`:
- Around line 198-205: SendMetas currently discards the context parameter;
update jsonBackend.SendMetas to honor the provided ctx by checking ctx.Err() (or
ctx.Done()) inside the metas.Walk callback (before reading the file and before
calling sender) and return early with the context error when cancelled; locate
the jsonBackend.SendMetas function and its anonymous callback passed to
q.metas.Walk, add a context cancellation check at the top of that callback and
propagate ctx.Err() (or wrap it) instead of continuing to use q.metaFile,
os.ReadFile, or sender when cancelled.
In `@staging/operator-registry/pkg/cache/pogrebv1.go`:
- Around line 214-221: The SendMetas function currently ignores its context (it
uses _ context.Context) so pogreb scanning continues after cancellation; change
the signature to use the provided ctx and in the q.metas.Walk callback check
ctx.Err() before doing work (e.g., at the top of the Walk func) and return
ctx.Err() to stop the walk when canceled; ensure this check happens before
q.db.Get(q.metaDBKey(key)) and before calling sender(key.Schema,
key.PackageName, key.Name, data) so DB reads and sends stop promptly.
In `@staging/operator-registry/pkg/server/server.go`:
- Around line 117-129: The fmt.Errorf calls inside the lambda passed to
mq.SendCustomSchemas are not wrapping errors with %w; update the error returns
in the anonymous function (the json.Unmarshal error and the structpb.NewStruct
error) to use fmt.Errorf with %w so callers can inspect the underlying error
(i.e., change the two fmt.Errorf("%v", err) uses to use a descriptive message
and %w, keeping the same surrounding text in the function passed to
mq.SendCustomSchemas that ultimately calls stream.Send).
---
Nitpick comments:
In `@staging/operator-registry/pkg/server/server_test.go`:
- Around line 812-815: Add an empty-request test run that targets the populated
custom-schema cache: in TestGetCustomSchemas call
testGetCustomSchemasEmpty(customSchemaCacheAddress) (in addition to the existing
cacheAddress case) and assert the response returns the two distinct resource
names expected; likewise update the related test runs referenced (the blocks
around lines 827–841 and 850–862) to invoke the empty-schema path against
customSchemaCacheAddress as well so the unfiltered RPC path is exercised and the
two resource names are asserted.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b75bca00-e245-487b-a36c-36f55ccac912
⛔ Files ignored due to path filters (10)
staging/operator-registry/pkg/api/registry.pb.gois excluded by!**/*.pb.gostaging/operator-registry/pkg/api/registry_grpc.pb.gois excluded by!**/*.pb.govendor/github.com/operator-framework/operator-registry/pkg/api/registry.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/api/registry.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/api/registry_grpc.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/meta_key.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/pogrebv1.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/server/server.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
staging/operator-registry/AGENTS.mdstaging/operator-registry/Makefilestaging/operator-registry/README.mdstaging/operator-registry/pkg/api/registry.protostaging/operator-registry/pkg/cache/cache.gostaging/operator-registry/pkg/cache/cache_test.gostaging/operator-registry/pkg/cache/json.gostaging/operator-registry/pkg/cache/meta_key.gostaging/operator-registry/pkg/cache/pogrebv1.gostaging/operator-registry/pkg/client/client_test.gostaging/operator-registry/pkg/server/server.gostaging/operator-registry/pkg/server/server_test.gostaging/operator-registry/scripts/ensure-protoc.sh
Comment on lines
+198
to
+205
| func (q *jsonBackend) SendMetas(_ context.Context, sender func(schema, pkg, name string, blob []byte) error) error { | ||
| return q.metas.Walk(func(key metaKey) error { | ||
| data, err := os.ReadFile(q.metaFile(key)) | ||
| if err != nil { | ||
| return fmt.Errorf("get meta %v: %w", key, err) | ||
| } | ||
| return sender(key.Schema, key.PackageName, key.Name, data) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code at the specified lines
head -n 210 staging/operator-registry/pkg/cache/json.go | tail -n 20Repository: openshift/operator-framework-olm
Length of output: 689
🏁 Script executed:
# Let's get more context around the function
sed -n '195,210p' staging/operator-registry/pkg/cache/json.goRepository: openshift/operator-framework-olm
Length of output: 575
🏁 Script executed:
# Let's find the Walk function definition to understand if it already supports context
rg "func.*Walk" staging/operator-registry/pkg/cache/json.go -A 3Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
# Check the type of metas field and its Walk method
rg "metas\s*:" staging/operator-registry/pkg/cache/json.go -B 5 -A 2Repository: openshift/operator-framework-olm
Length of output: 229
🏁 Script executed:
# Look for the entire jsonBackend struct
ast-grep --pattern 'type jsonBackend struct { $$$ }'Repository: openshift/operator-framework-olm
Length of output: 855
🏁 Script executed:
# Find metaKeys type definition
rg "type metaKeys" staging/operator-registry/pkg/cache/json.go -A 5Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
# Search for Walk method definition on metaKeys
rg "func.*metaKeys.*Walk" staging/operator-registry/pkg/cache/json.go -A 8Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
# More general search for Walk method
rg "func.*\(.*metaKeys\).*Walk" staging/operator-registry/pkg/cache/json.go -A 8Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
# Search for any Walk method in the file
rg "func.*Walk" staging/operator-registry/pkg/cache/ -A 5Repository: openshift/operator-framework-olm
Length of output: 996
🏁 Script executed:
# Get full implementation of metaKeys.Walk
sed -n '/^func (m \*metaKeys) Walk/,/^func /p' staging/operator-registry/pkg/cache/meta_key.go | head -n 20Repository: openshift/operator-framework-olm
Length of output: 364
🏁 Script executed:
# Get full implementation of bundleKeys.Walk
sed -n '/^func (b bundleKeys) Walk/,/^func /p' staging/operator-registry/pkg/cache/bundle_key.go | head -n 20Repository: openshift/operator-framework-olm
Length of output: 245
🏁 Script executed:
# Check if there are similar Send* functions in other backends (file.go, sql.go, etc)
fd "\\.go$" staging/operator-registry/pkg/cache/ --exec grep -l "Send" {} \;Repository: openshift/operator-framework-olm
Length of output: 263
🏁 Script executed:
# Look for other Send* functions that accept context
rg "func.*Send.*context.Context" staging/operator-registry/pkg/cache/ -A 3Repository: openshift/operator-framework-olm
Length of output: 2542
🏁 Script executed:
# Find the interface that defines SendMetas
rg "SendMetas" staging/operator-registry/pkg/cache/ -B 3 | grep -E "(interface|type.*struct)" -A 10Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
# Search for Backend interface definition
rg "type.*Backend" staging/operator-registry/pkg/cache/ | head -20Repository: openshift/operator-framework-olm
Length of output: 224
🏁 Script executed:
# Look at cache.go to see the Backend interface
sed -n '1,100p' staging/operator-registry/pkg/cache/cache.go | grep -A 30 "interface"Repository: openshift/operator-framework-olm
Length of output: 1281
🏁 Script executed:
# Search for Backend interface in cache.go more thoroughly
rg "interface" staging/operator-registry/pkg/cache/cache.go -A 20Repository: openshift/operator-framework-olm
Length of output: 1091
🏁 Script executed:
# Check SendBundles implementation in json.go
sed -n '/func (q \*jsonBackend) SendBundles/,/^func /p' staging/operator-registry/pkg/cache/json.go | head -30Repository: openshift/operator-framework-olm
Length of output: 1070
🏁 Script executed:
# Check SendBundles implementation in pogrebv1.go
sed -n '/func (q \*pogrebV1Backend) SendBundles/,/^func /p' staging/operator-registry/pkg/cache/pogrebv1.go | head -30Repository: openshift/operator-framework-olm
Length of output: 670
Use the context parameter to cancel iteration on context cancellation.
The SendMetas method accepts context.Context per the interface contract but explicitly discards it with _. Since this function iterates over potentially many metadata blobs and reads each from disk, it should respect context cancellation—particularly when backing streamed RPCs. Add context error checks in the loop to abort early if the caller cancels.
Suggested change
-func (q *jsonBackend) SendMetas(_ context.Context, sender func(schema, pkg, name string, blob []byte) error) error {
+func (q *jsonBackend) SendMetas(ctx context.Context, sender func(schema, pkg, name string, blob []byte) error) error {
return q.metas.Walk(func(key metaKey) error {
+ if err := ctx.Err(); err != nil {
+ return err
+ }
data, err := os.ReadFile(q.metaFile(key))
if err != nil {
return fmt.Errorf("get meta %v: %w", key, err)
}
+ if err := ctx.Err(); err != nil {
+ return err
+ }
return sender(key.Schema, key.PackageName, key.Name, data)
})
}🤖 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 `@staging/operator-registry/pkg/cache/json.go` around lines 198 - 205,
SendMetas currently discards the context parameter; update jsonBackend.SendMetas
to honor the provided ctx by checking ctx.Err() (or ctx.Done()) inside the
metas.Walk callback (before reading the file and before calling sender) and
return early with the context error when cancelled; locate the
jsonBackend.SendMetas function and its anonymous callback passed to
q.metas.Walk, add a context cancellation check at the top of that callback and
propagate ctx.Err() (or wrap it) instead of continuing to use q.metaFile,
os.ReadFile, or sender when cancelled.
Comment on lines
+214
to
+221
| func (q *pogrebV1Backend) SendMetas(_ context.Context, sender func(schema, pkg, name string, blob []byte) error) error { | ||
| return q.metas.Walk(func(key metaKey) error { | ||
| data, err := q.db.Get(q.metaDBKey(key)) | ||
| if err != nil { | ||
| return fmt.Errorf("get meta %v: %w", key, err) | ||
| } | ||
| return sender(key.Schema, key.PackageName, key.Name, data) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/pogrebv1.go | head -250 | tail -50Repository: openshift/operator-framework-olm
Length of output: 1733
🏁 Script executed:
wc -l staging/operator-registry/pkg/cache/pogrebv1.goRepository: openshift/operator-framework-olm
Length of output: 128
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/pogrebv1.go | sed -n '200,230p'Repository: openshift/operator-framework-olm
Length of output: 1226
🏁 Script executed:
rg -A 10 "func.*Walk" staging/operator-registry/pkg/cache/pogrebv1.goRepository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg "func.*Backend.*Send" staging/operator-registry/pkg/cache/pogrebv1.goRepository: openshift/operator-framework-olm
Length of output: 287
🏁 Script executed:
fd "*.go" staging/operator-registry/pkg/cache/ | head -10Repository: openshift/operator-framework-olm
Length of output: 360
🏁 Script executed:
rg -A 8 "func.*SendMetas" staging/operator-registry/pkg/cache/Repository: openshift/operator-framework-olm
Length of output: 1603
🏁 Script executed:
rg -B 5 -A 15 "func (q \*pogrebV1Backend) SendMetas" staging/operator-registry/pkg/cache/pogrebv1.goRepository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg "\.Walk\(" staging/operator-registry/pkg/cache/pogrebv1.goRepository: openshift/operator-framework-olm
Length of output: 290
🏁 Script executed:
rg -B 2 -A 10 "metas\.Walk" staging/operator-registry/pkg/cache/pogrebv1.goRepository: openshift/operator-framework-olm
Length of output: 567
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/pogrebv1.go | sed -n '1,50p'Repository: openshift/operator-framework-olm
Length of output: 1404
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/pogrebv1.go | sed -n '100,150p'Repository: openshift/operator-framework-olm
Length of output: 1661
🏁 Script executed:
rg "metas" staging/operator-registry/pkg/cache/pogrebv1.go | head -20Repository: openshift/operator-framework-olm
Length of output: 321
🏁 Script executed:
rg -B 5 -A 10 "type metaKeys" staging/operator-registry/pkg/cache/Repository: openshift/operator-framework-olm
Length of output: 1128
🏁 Script executed:
rg -B 2 -A 15 "func.*metaKeys.*Walk" staging/operator-registry/pkg/cache/Repository: openshift/operator-framework-olm
Length of output: 1231
🏁 Script executed:
rg -B 2 -A 15 "func.*SendBundles" staging/operator-registry/pkg/cache/pogrebv1.goRepository: openshift/operator-framework-olm
Length of output: 673
🏁 Script executed:
rg -B 2 -A 15 "func.*SendBundles" staging/operator-registry/pkg/cache/json.goRepository: openshift/operator-framework-olm
Length of output: 697
🏁 Script executed:
rg "ctx\.Err\(\)" staging/operator-registry/pkg/cache/Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg "func.*Send" staging/operator-registry/pkg/cache/ | grep -v "BundleSender\|MetaSender"Repository: openshift/operator-framework-olm
Length of output: 932
🏁 Script executed:
rg "func.*Send.*_ context.Context" staging/operator-registry/pkg/cache/Repository: openshift/operator-framework-olm
Length of output: 674
🏁 Script executed:
rg -B 5 "SendMetas\|SendBundles" staging/operator-registry/pkg/cache/backend.goRepository: openshift/operator-framework-olm
Length of output: 163
🏁 Script executed:
rg -B 3 -A 10 "type backend interface" staging/operator-registry/pkg/cache/Repository: openshift/operator-framework-olm
Length of output: 1066
🏁 Script executed:
rg -B 3 -A 3 "select.*ctx\.Done\|if.*ctx\.Err" staging/operator-registry/pkg/ | head -40Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
fd registry.go staging/operator-registry/pkg/ | head -3Repository: openshift/operator-framework-olm
Length of output: 280
🏁 Script executed:
rg "SendMetas\|SendBundles" staging/operator-registry/pkg/registry/ | head -20Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg -A 30 "type backend interface" staging/operator-registry/pkg/cache/cache.goRepository: openshift/operator-framework-olm
Length of output: 948
🏁 Script executed:
rg -B 5 -A 8 "SendMetas" staging/operator-registry/pkg/registry/ | head -50Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg "\.SendMetas\|\.SendBundles" staging/operator-registry/ --type go | head -30Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg "SendMetas\|SendBundles" staging/operator-registry/ --type go | grep -E "test\.go|example|handler" | head -20Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
fd "service.pb.go\|service.go" staging/operator-registry/pkg/api/ | head -5Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg "Metas\|Bundles" staging/operator-registry/pkg/api/ | grep "func\|Sender" | head -20Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg "ListBundles\|GetBundles" staging/operator-registry/pkg/ --type go | head -10Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
fd "\.proto" staging/operator-registry/Repository: openshift/operator-framework-olm
Length of output: 125
🏁 Script executed:
rg "rpc.*Meta\|rpc.*Bundle" staging/operator-registry/ | head -20Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg -B 5 -A 10 "SendMetas\|SendBundles" staging/operator-registry/pkg/cache/cache.goRepository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
rg "backend\|\.SendMetas\|\.SendBundles" staging/operator-registry/pkg/cache/cache.go | head -40Repository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/cache.go | head -100Repository: openshift/operator-framework-olm
Length of output: 3390
🏁 Script executed:
rg "func.*cache.*Send" staging/operator-registry/pkg/cache/cache.goRepository: openshift/operator-framework-olm
Length of output: 318
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/json.go | sed -n '140,180p'Repository: openshift/operator-framework-olm
Length of output: 1459
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/pogrebv1.go | sed -n '260,290p'Repository: openshift/operator-framework-olm
Length of output: 1187
🏁 Script executed:
rg -B 5 -A 10 "c\.backend\.SendMetas\|backend\.SendMetas" staging/operator-registry/pkg/cache/cache.goRepository: openshift/operator-framework-olm
Length of output: 58
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/cache.go | sed -n '200,300p'Repository: openshift/operator-framework-olm
Length of output: 4497
🏁 Script executed:
rg "func.*cache.*" staging/operator-registry/pkg/cache/cache.go | head -20Repository: openshift/operator-framework-olm
Length of output: 1677
🏁 Script executed:
cat -n staging/operator-registry/pkg/cache/cache.go | sed -n '251,264p'Repository: openshift/operator-framework-olm
Length of output: 606
Honor cancellation while scanning Pogreb meta entries.
SendMetas keeps reading from Pogreb even after the request context is canceled. For a streamed gRPC path, that means unnecessary DB work after the client is already gone.
Suggested change
-func (q *pogrebV1Backend) SendMetas(_ context.Context, sender func(schema, pkg, name string, blob []byte) error) error {
+func (q *pogrebV1Backend) SendMetas(ctx context.Context, sender func(schema, pkg, name string, blob []byte) error) error {
return q.metas.Walk(func(key metaKey) error {
+ if err := ctx.Err(); err != nil {
+ return err
+ }
data, err := q.db.Get(q.metaDBKey(key))
if err != nil {
return fmt.Errorf("get meta %v: %w", key, err)
}
+ if err := ctx.Err(); err != nil {
+ return err
+ }
return sender(key.Schema, key.PackageName, key.Name, data)
})
}📝 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 (q *pogrebV1Backend) SendMetas(_ context.Context, sender func(schema, pkg, name string, blob []byte) error) error { | |
| return q.metas.Walk(func(key metaKey) error { | |
| data, err := q.db.Get(q.metaDBKey(key)) | |
| if err != nil { | |
| return fmt.Errorf("get meta %v: %w", key, err) | |
| } | |
| return sender(key.Schema, key.PackageName, key.Name, data) | |
| }) | |
| func (q *pogrebV1Backend) SendMetas(ctx context.Context, sender func(schema, pkg, name string, blob []byte) error) error { | |
| return q.metas.Walk(func(key metaKey) error { | |
| if err := ctx.Err(); err != nil { | |
| return err | |
| } | |
| data, err := q.db.Get(q.metaDBKey(key)) | |
| if err != nil { | |
| return fmt.Errorf("get meta %v: %w", key, err) | |
| } | |
| if err := ctx.Err(); err != nil { | |
| return err | |
| } | |
| return sender(key.Schema, key.PackageName, key.Name, data) | |
| }) | |
| } |
🤖 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 `@staging/operator-registry/pkg/cache/pogrebv1.go` around lines 214 - 221, The
SendMetas function currently ignores its context (it uses _ context.Context) so
pogreb scanning continues after cancellation; change the signature to use the
provided ctx and in the q.metas.Walk callback check ctx.Err() before doing work
(e.g., at the top of the Walk func) and return ctx.Err() to stop the walk when
canceled; ensure this check happens before q.db.Get(q.metaDBKey(key)) and before
calling sender(key.Schema, key.PackageName, key.Name, data) so DB reads and
sends stop promptly.
Comment on lines
+117
to
+129
| return mq.SendCustomSchemas(stream.Context(), req.GetSchema(), req.GetPackageName(), req.GetName(), | ||
| func(_, _, _ string, blob []byte) error { | ||
| var m map[string]interface{} | ||
| if err := json.Unmarshal(blob, &m); err != nil { | ||
| return fmt.Errorf("unmarshal custom schema blob: %v", err) | ||
| } | ||
| s, err := structpb.NewStruct(m) | ||
| if err != nil { | ||
| return fmt.Errorf("convert custom schema blob to struct: %v", err) | ||
| } | ||
| return stream.Send(s) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Wrap errors with %w for proper error chain propagation.
Lines 121 and 125 use fmt.Errorf with %v instead of %w. This breaks the error chain and prevents callers from using errors.Is or errors.As to inspect the underlying error.
🔧 Proposed fix
var m map[string]interface{}
if err := json.Unmarshal(blob, &m); err != nil {
- return fmt.Errorf("unmarshal custom schema blob: %v", err)
+ return fmt.Errorf("unmarshal custom schema blob: %w", err)
}
s, err := structpb.NewStruct(m)
if err != nil {
- return fmt.Errorf("convert custom schema blob to struct: %v", err)
+ return fmt.Errorf("convert custom schema blob to struct: %w", err)
}As per coding guidelines: "Wrap errors with context using fmt.Errorf() with the %w verb in Go code" for files matching staging/operator-registry/**/*.go.
📝 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
| return mq.SendCustomSchemas(stream.Context(), req.GetSchema(), req.GetPackageName(), req.GetName(), | |
| func(_, _, _ string, blob []byte) error { | |
| var m map[string]interface{} | |
| if err := json.Unmarshal(blob, &m); err != nil { | |
| return fmt.Errorf("unmarshal custom schema blob: %v", err) | |
| } | |
| s, err := structpb.NewStruct(m) | |
| if err != nil { | |
| return fmt.Errorf("convert custom schema blob to struct: %v", err) | |
| } | |
| return stream.Send(s) | |
| }) | |
| } | |
| return mq.SendCustomSchemas(stream.Context(), req.GetSchema(), req.GetPackageName(), req.GetName(), | |
| func(_, _, _ string, blob []byte) error { | |
| var m map[string]interface{} | |
| if err := json.Unmarshal(blob, &m); err != nil { | |
| return fmt.Errorf("unmarshal custom schema blob: %w", err) | |
| } | |
| s, err := structpb.NewStruct(m) | |
| if err != nil { | |
| return fmt.Errorf("convert custom schema blob to struct: %w", err) | |
| } | |
| return stream.Send(s) | |
| }) | |
| } |
🤖 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 `@staging/operator-registry/pkg/server/server.go` around lines 117 - 129, The
fmt.Errorf calls inside the lambda passed to mq.SendCustomSchemas are not
wrapping errors with %w; update the error returns in the anonymous function (the
json.Unmarshal error and the structpb.NewStruct error) to use fmt.Errorf with %w
so callers can inspect the underlying error (i.e., change the two
fmt.Errorf("%v", err) uses to use a descriptive message and %w, keeping the same
surrounding text in the function passed to mq.SendCustomSchemas that ultimately
calls stream.Send).
perdasilva
changed the title
perdasilva
force-pushed
the
opm-endpoint
branch
2 times, most recently
from
9ceb485 to
af48969
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
staging/operator-registry/pkg/server/server.go (1)
122-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap these new errors with
%winstead of%v.Current wrapping drops the causal chain from
json.Unmarshalandstructpb.NewStruct.As per coding guidelines: "Error handling must wrap errors with context using fmt.Errorf with %w verb."
🤖 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 `@staging/operator-registry/pkg/server/server.go` around lines 122 - 127, The error returns after json.Unmarshal and structpb.NewStruct should wrap the underlying error with %w so the causal chain is preserved; update the two fmt.Errorf calls that currently use "%v" to use "%w" (e.g., in the block handling json.Unmarshal(blob, &m) and the subsequent structpb.NewStruct(m) call) so both return fmt.Errorf("<context>: %w", err) instead of "%v".staging/operator-registry/pkg/cache/json.go (1)
162-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor context cancellation while streaming metas.
SendMetascurrently discardsctx, so it continues disk I/O and sending after caller cancellation.As per coding guidelines: "Always pass context to long-running operations in function signatures and calls."
🤖 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 `@staging/operator-registry/pkg/cache/json.go` around lines 162 - 184, SendMetas ignores ctx and can continue heavy I/O after cancellation; modify jsonBackend.SendMetas to respect the provided context by checking ctx.Done()/ctx.Err() at key points (before iterating, before each os.ReadFile call, and before invoking sender) and return early with ctx.Err() when cancelled; ensure you still treat os.ErrNotExist the same way when calling os.ReadDir(dir) and keep existing filtering logic (entry.IsDir() and filepath.Ext), but abort and propagate the context error instead of continuing I/O or calling sender when the context is cancelled.staging/operator-registry/pkg/cache/pogrebv1.go (1)
181-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRespect
ctxinSendMetasiteration.The method currently keeps scanning/sending after cancellation because
ctxis ignored.As per coding guidelines: "Always pass context to long-running operations in function signatures and calls."
🤖 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 `@staging/operator-registry/pkg/cache/pogrebv1.go` around lines 181 - 194, SendMetas ignores the provided ctx and can continue long-running iteration after cancellation; modify pogrebV1Backend.SendMetas to check ctx cancellation on each loop iteration (e.g., select on ctx.Done()) and return ctx.Err() when cancelled, and where possible pass ctx into any underlying long-running calls (e.g., any db.Get variants that accept context) or bail out before calling sender; ensure checks are performed before/after calling q.db.Get and before invoking sender so iteration stops promptly when ctx is done.
🤖 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 `@staging/operator-registry/pkg/cache/cache.go`:
- Around line 245-247: ListPackageCustomSchemas currently forwards raw schema
and packageName into backend.SendMetas which risks unsafe filesystem keys; call
newValidatedMetaKey(schema, packageName) inside ListPackageCustomSchemas, check
and return any error from newValidatedMetaKey, and then pass the returned
validated metaKey into c.backend.SendMetas instead of constructing metaKey
directly so inputs are validated/sanitized before the backend lookup.
In `@staging/operator-registry/pkg/cache/json.go`:
- Around line 149-159: PutMeta in jsonBackend currently computes the next
filename using len(entries), which can collide under concurrent writers; change
the implementation to create a unique temporary file in the target directory
(e.g., using os.CreateTemp or equivalent) and write the blob to that temp file
and close it (optionally os.Chmod to jsonCacheModeFile), which guarantees a
unique name and atomic write; update function jsonBackend.PutMeta to replace the
len(entries)-based filename with this temp-file approach so concurrent calls
cannot clobber each other.
In `@staging/operator-registry/pkg/cache/pogrebv1.go`:
- Around line 174-179: PutMeta updates to the pogrebV1Backend.metaCounts map are
not protected and can cause concurrent map writes; add concurrency control by
introducing a mutex on pogrebV1Backend (e.g., metaMu sync.Mutex or sync.RWMutex)
and lock around the read-modify-write sequence in PutMeta (use
metaMu.Lock()/Unlock() while calling metaKeyPrefix, reading
q.metaCounts[prefix], incrementing and storing it) and update any other sites
that access q.metaCounts to use the same mutex to ensure safe concurrent access.
In `@staging/operator-registry/pkg/server/server_test.go`:
- Around line 876-882: The test currently returns early when
ListPackageCustomSchemas returns an error, which skips assertions; update the
test around c.ListPackageCustomSchemas(context.TODO(),
&api.ListPackageCustomSchemasRequest{}) so it does not return on initial err but
instead asserts that the call either returns a non-nil stream and that the
subsequent stream.Recv() yields an error, or if the initial call returns an
error assert that error is non-nil and has the expected gRPC code; specifically
replace the early `return` with assertions using require.NoError/require.Error
as appropriate and check the gRPC status code (e.g., status.Code(err) ==
codes.InvalidArgument) when verifying the error from ListPackageCustomSchemas or
from stream.Recv().
---
Duplicate comments:
In `@staging/operator-registry/pkg/cache/json.go`:
- Around line 162-184: SendMetas ignores ctx and can continue heavy I/O after
cancellation; modify jsonBackend.SendMetas to respect the provided context by
checking ctx.Done()/ctx.Err() at key points (before iterating, before each
os.ReadFile call, and before invoking sender) and return early with ctx.Err()
when cancelled; ensure you still treat os.ErrNotExist the same way when calling
os.ReadDir(dir) and keep existing filtering logic (entry.IsDir() and
filepath.Ext), but abort and propagate the context error instead of continuing
I/O or calling sender when the context is cancelled.
In `@staging/operator-registry/pkg/cache/pogrebv1.go`:
- Around line 181-194: SendMetas ignores the provided ctx and can continue
long-running iteration after cancellation; modify pogrebV1Backend.SendMetas to
check ctx cancellation on each loop iteration (e.g., select on ctx.Done()) and
return ctx.Err() when cancelled, and where possible pass ctx into any underlying
long-running calls (e.g., any db.Get variants that accept context) or bail out
before calling sender; ensure checks are performed before/after calling q.db.Get
and before invoking sender so iteration stops promptly when ctx is done.
In `@staging/operator-registry/pkg/server/server.go`:
- Around line 122-127: The error returns after json.Unmarshal and
structpb.NewStruct should wrap the underlying error with %w so the causal chain
is preserved; update the two fmt.Errorf calls that currently use "%v" to use
"%w" (e.g., in the block handling json.Unmarshal(blob, &m) and the subsequent
structpb.NewStruct(m) call) so both return fmt.Errorf("<context>: %w", err)
instead of "%v".
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2fc45da7-8a42-41db-9eab-0fa1339fe253
⛔ Files ignored due to path filters (10)
staging/operator-registry/pkg/api/registry.pb.gois excluded by!**/*.pb.gostaging/operator-registry/pkg/api/registry_grpc.pb.gois excluded by!**/*.pb.govendor/github.com/operator-framework/operator-registry/pkg/api/registry.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/api/registry.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/api/registry_grpc.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/meta_key.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/pogrebv1.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/server/server.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
staging/operator-registry/AGENTS.mdstaging/operator-registry/Makefilestaging/operator-registry/README.mdstaging/operator-registry/pkg/api/registry.protostaging/operator-registry/pkg/cache/cache.gostaging/operator-registry/pkg/cache/cache_test.gostaging/operator-registry/pkg/cache/json.gostaging/operator-registry/pkg/cache/meta_key.gostaging/operator-registry/pkg/cache/pogrebv1.gostaging/operator-registry/pkg/client/client_test.gostaging/operator-registry/pkg/server/server.gostaging/operator-registry/pkg/server/server_test.gostaging/operator-registry/scripts/ensure-protoc.sh
✅ Files skipped from review due to trivial changes (1)
- staging/operator-registry/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- staging/operator-registry/AGENTS.md
- staging/operator-registry/scripts/ensure-protoc.sh
- staging/operator-registry/Makefile
- staging/operator-registry/pkg/client/client_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
staging/operator-registry/pkg/cache/json.go (1)
161-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor context cancellation while streaming metas.
SendMetasstill discardscontext.Context, so cancelled requests continue reading files and invoking callbacks.Suggested fix
-func (q *jsonBackend) SendMetas(_ context.Context, key metaKey, sender func([]byte) error) error { +func (q *jsonBackend) SendMetas(ctx context.Context, key metaKey, sender func([]byte) error) error { dir := q.metaDir(key) for i := 0; ; i++ { + if err := ctx.Err(); err != nil { + return err + } data, err := os.ReadFile(filepath.Join(dir, fmt.Sprintf("%d.json", i))) if err != nil { if errors.Is(err, os.ErrNotExist) { return nil } return err } + if err := ctx.Err(); err != nil { + return err + } if err := sender(data); err != nil { return err } } }As per coding guidelines
staging/operator-registry/**/*.go: "Use context.Context parameter for all long-running operations and async functions".🤖 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 `@staging/operator-registry/pkg/cache/json.go` around lines 161 - 175, SendMetas on jsonBackend ignores the provided context so cancelled callers still read files and call sender; modify SendMetas(ctx context.Context, key metaKey, sender func([]byte) error) to respect ctx by checking ctx.Done() in the loop (use a select before each file read and before calling sender) and return ctx.Err() when cancelled, and propagate any sender error as before; ensure you reference the jsonBackend.SendMetas method and the metaKey/sender parameters when making the change.
🧹 Nitpick comments (1)
staging/operator-registry/pkg/cache/cache_test.go (1)
267-300: ⚡ Quick winRefactor scenario assertions into table-driven subtests.
This test packs multiple scenarios into one block; converting those cases to a table with
t.Runwould match the repository’s unit-test guideline and make failures more targeted.As per coding guidelines
staging/operator-registry/**/*_test.go: "Use table-driven tests with t.Run() subtests for unit tests".🤖 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 `@staging/operator-registry/pkg/cache/cache_test.go` around lines 267 - 300, Refactor TestCache_ListPackageCustomSchemas into a table-driven subtest: inside the existing loop over genTestCaches(t, customSchemaFS) and the current t.Run(name...), keep the helper collect(schema, pkg) but replace the inline assertions with a slice of test cases (e.g., cases for "multiple blobs" {schema:"custom.operator.io", pkg:"testpkg", wantLen:2}, "different schema" {schema:"other.custom.schema", pkg:"testpkg", wantLen:1, wantContains:`"info"`}, "nonexistent schema" {schema:"nonexistent.schema", pkg:"testpkg", wantEmpty:true}, "nonexistent package" {schema:"custom.operator.io", pkg:"nonexistent", wantEmpty:true}) and iterate cases with t.Run(caseName, func(t *testing.T){ blobs := collect(case.schema, case.pkg); assert expected conditions using require.Len/require.Contains/require.Empty accordingly }); keep references to TestCache_ListPackageCustomSchemas, collect, genTestCaches and ListPackageCustomSchemas to locate the code.
🤖 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 `@staging/operator-registry/pkg/cache/json.go`:
- Around line 151-159: The PutMeta function advances q.metaCounts[dir] before
the write succeeds, which can create gaps if os.WriteFile fails; change the flow
in jsonBackend.PutMeta to compute idx := q.metaCounts[dir], attempt os.WriteFile
with filepath.Join(dir, fmt.Sprintf("%d.json", idx)) first, and only if that
call returns nil set q.metaCounts[dir] = idx + 1 (and still return the write
error if any). Ensure you keep the os.MkdirAll step before the write and use the
same idx variable/reference to q.metaCounts to avoid races or off-by-one
updates.
---
Duplicate comments:
In `@staging/operator-registry/pkg/cache/json.go`:
- Around line 161-175: SendMetas on jsonBackend ignores the provided context so
cancelled callers still read files and call sender; modify SendMetas(ctx
context.Context, key metaKey, sender func([]byte) error) to respect ctx by
checking ctx.Done() in the loop (use a select before each file read and before
calling sender) and return ctx.Err() when cancelled, and propagate any sender
error as before; ensure you reference the jsonBackend.SendMetas method and the
metaKey/sender parameters when making the change.
---
Nitpick comments:
In `@staging/operator-registry/pkg/cache/cache_test.go`:
- Around line 267-300: Refactor TestCache_ListPackageCustomSchemas into a
table-driven subtest: inside the existing loop over genTestCaches(t,
customSchemaFS) and the current t.Run(name...), keep the helper collect(schema,
pkg) but replace the inline assertions with a slice of test cases (e.g., cases
for "multiple blobs" {schema:"custom.operator.io", pkg:"testpkg", wantLen:2},
"different schema" {schema:"other.custom.schema", pkg:"testpkg", wantLen:1,
wantContains:`"info"`}, "nonexistent schema" {schema:"nonexistent.schema",
pkg:"testpkg", wantEmpty:true}, "nonexistent package"
{schema:"custom.operator.io", pkg:"nonexistent", wantEmpty:true}) and iterate
cases with t.Run(caseName, func(t *testing.T){ blobs := collect(case.schema,
case.pkg); assert expected conditions using
require.Len/require.Contains/require.Empty accordingly }); keep references to
TestCache_ListPackageCustomSchemas, collect, genTestCaches and
ListPackageCustomSchemas to locate the code.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c7e1eda6-436b-4994-a400-53d27baa35fa
⛔ Files ignored due to path filters (10)
staging/operator-registry/pkg/api/registry.pb.gois excluded by!**/*.pb.gostaging/operator-registry/pkg/api/registry_grpc.pb.gois excluded by!**/*.pb.govendor/github.com/operator-framework/operator-registry/pkg/api/registry.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/api/registry.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/api/registry_grpc.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/meta_key.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/pogrebv1.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/server/server.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
staging/operator-registry/AGENTS.mdstaging/operator-registry/Makefilestaging/operator-registry/README.mdstaging/operator-registry/pkg/api/registry.protostaging/operator-registry/pkg/cache/cache.gostaging/operator-registry/pkg/cache/cache_test.gostaging/operator-registry/pkg/cache/json.gostaging/operator-registry/pkg/cache/meta_key.gostaging/operator-registry/pkg/cache/pogrebv1.gostaging/operator-registry/pkg/client/client_test.gostaging/operator-registry/pkg/server/server.gostaging/operator-registry/pkg/server/server_test.gostaging/operator-registry/scripts/ensure-protoc.sh
✅ Files skipped from review due to trivial changes (3)
- staging/operator-registry/scripts/ensure-protoc.sh
- staging/operator-registry/README.md
- staging/operator-registry/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (8)
- staging/operator-registry/pkg/api/registry.proto
- staging/operator-registry/pkg/cache/meta_key.go
- staging/operator-registry/Makefile
- staging/operator-registry/pkg/server/server.go
- staging/operator-registry/pkg/client/client_test.go
- staging/operator-registry/pkg/cache/pogrebv1.go
- staging/operator-registry/pkg/server/server_test.go
- staging/operator-registry/pkg/cache/cache.go
…serve Add a streaming gRPC endpoint that allows callers to retrieve raw FBC blobs for non-standard (custom) schemas, scoped to a specific package. Both schema and packageName are required parameters. The endpoint streams all matching blobs as google.protobuf.Struct, leaving it to the caller — who understands the custom schema's semantics — to handle filtering, deduplication, or validation of the results. Standard schemas (olm.package, olm.channel, olm.bundle, olm.deprecations) are excluded since they are already served by existing typed RPCs. Custom schema blobs without a package field are silently skipped during cache build, as they fall outside the scope of this package-scoped endpoint. Custom schema blobs are stored in the cache using a two-field key (schema, packageName) with sequential indices to support multiple blobs per key, backed by both pogreb and JSON cache backends. Signed-off-by: Per G. da Silva <pegoncal@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
staging/operator-registry/pkg/cache/pogrebv1.go (1)
183-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor cancellation while iterating meta blobs.
At Line 183,
ctxis discarded, so Line 186-199 can continue iterating/reading after caller cancellation. Please checkctx.Err()inside the loop before DB work and beforesender(...)to stop promptly.Suggested minimal fix
-func (q *pogrebV1Backend) SendMetas(_ context.Context, key metaKey, sender func([]byte) error) error { +func (q *pogrebV1Backend) SendMetas(ctx context.Context, key metaKey, sender func([]byte) error) error { prefix := []byte(q.metaDBKeyPrefix(key)) it := q.db.Items() for { + if err := ctx.Err(); err != nil { + return err + } k, v, err := it.Next() if errors.Is(err, pogreb.ErrIterationDone) { return nil } if err != nil { return err } if bytes.HasPrefix(k, prefix) { + if err := ctx.Err(); err != nil { + return err + } if err := sender(v); err != nil { return err } } } }As per coding guidelines: "Use context.Context for long-running operations and always pass context to functions that need it".
🤖 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 `@staging/operator-registry/pkg/cache/pogrebv1.go` around lines 183 - 199, The SendMetas method (pogrebV1Backend.SendMetas) currently ignores the provided ctx and can continue work after cancellation; update the loop to check ctx.Err() at the top of each iteration and again immediately before calling sender(v) to return promptly on cancellation, i.e. if ctx.Err() != nil return ctx.Err(); ensure you reference the metaKey/prefix logic and preserve existing error handling from the DB iterator while adding these context checks.staging/operator-registry/pkg/cache/json.go (1)
159-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
ctxto stop meta file scanning on cancellation.At Line 159,
ctxis intentionally ignored. For this disk-iteration path, cancellation should short-circuit before further reads and before invokingsender.Suggested minimal fix
-func (q *jsonBackend) SendMetas(_ context.Context, key metaKey, sender func([]byte) error) error { +func (q *jsonBackend) SendMetas(ctx context.Context, key metaKey, sender func([]byte) error) error { dir := q.metaDir(key) entries, err := os.ReadDir(dir) if err != nil { if errors.Is(err, os.ErrNotExist) { return nil } return err } for _, entry := range entries { + if err := ctx.Err(); err != nil { + return err + } if entry.IsDir() || filepath.Ext(entry.Name()) != ".json" { continue } data, err := os.ReadFile(filepath.Join(dir, entry.Name())) if err != nil { return err } + if err := ctx.Err(); err != nil { + return err + } if err := sender(data); err != nil { return err } } return nil }As per coding guidelines: "Use context.Context for long-running operations and always pass context to functions that need it".
🤖 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 `@staging/operator-registry/pkg/cache/json.go` around lines 159 - 181, In jsonBackend.SendMetas, stop ignoring the provided ctx: check ctx.Done() before starting the per-file loop (after ReadDir) and inside the loop before reading each file and before invoking sender so cancellation short-circuits further work; locate the function jsonBackend.SendMetas, the metaDir() call that produces dir, the entries loop over os.ReadDir results, and add non-blocking context checks (select on ctx.Done()) to return ctx.Err() when cancelled instead of continuing reads or calling sender.
🤖 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 `@staging/operator-registry/pkg/cache/cache.go`:
- Around line 245-249: Replace the fmt.Errorf uses that currently use %v for
error values with %w so the error chain is preserved; specifically update the
error construction in ListPackageCustomSchemas (change "invalid custom schema
query: %v" to use %w), and the other two error returns that read "invalid custom
schema meta: %v" and "store custom schema meta %v: %v" so the final %v for the
underlying error becomes %w, ensuring callers can use errors.Is()/errors.As() to
inspect root causes.
---
Duplicate comments:
In `@staging/operator-registry/pkg/cache/json.go`:
- Around line 159-181: In jsonBackend.SendMetas, stop ignoring the provided ctx:
check ctx.Done() before starting the per-file loop (after ReadDir) and inside
the loop before reading each file and before invoking sender so cancellation
short-circuits further work; locate the function jsonBackend.SendMetas, the
metaDir() call that produces dir, the entries loop over os.ReadDir results, and
add non-blocking context checks (select on ctx.Done()) to return ctx.Err() when
cancelled instead of continuing reads or calling sender.
In `@staging/operator-registry/pkg/cache/pogrebv1.go`:
- Around line 183-199: The SendMetas method (pogrebV1Backend.SendMetas)
currently ignores the provided ctx and can continue work after cancellation;
update the loop to check ctx.Err() at the top of each iteration and again
immediately before calling sender(v) to return promptly on cancellation, i.e. if
ctx.Err() != nil return ctx.Err(); ensure you reference the metaKey/prefix logic
and preserve existing error handling from the DB iterator while adding these
context checks.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5bc037e7-fcf9-45de-ae1b-7c82f7677e30
⛔ Files ignored due to path filters (10)
staging/operator-registry/pkg/api/registry.pb.gois excluded by!**/*.pb.gostaging/operator-registry/pkg/api/registry_grpc.pb.gois excluded by!**/*.pb.govendor/github.com/operator-framework/operator-registry/pkg/api/registry.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/api/registry.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/api/registry_grpc.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/meta_key.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/cache/pogrebv1.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/operator-framework/operator-registry/pkg/server/server.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
staging/operator-registry/AGENTS.mdstaging/operator-registry/Makefilestaging/operator-registry/README.mdstaging/operator-registry/pkg/api/registry.protostaging/operator-registry/pkg/cache/cache.gostaging/operator-registry/pkg/cache/cache_test.gostaging/operator-registry/pkg/cache/json.gostaging/operator-registry/pkg/cache/meta_key.gostaging/operator-registry/pkg/cache/pogrebv1.gostaging/operator-registry/pkg/client/client_test.gostaging/operator-registry/pkg/server/server.gostaging/operator-registry/pkg/server/server_test.gostaging/operator-registry/scripts/ensure-protoc.sh
✅ Files skipped from review due to trivial changes (2)
- staging/operator-registry/AGENTS.md
- staging/operator-registry/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- staging/operator-registry/pkg/client/client_test.go
- staging/operator-registry/scripts/ensure-protoc.sh
- staging/operator-registry/pkg/cache/meta_key.go
- staging/operator-registry/Makefile
- staging/operator-registry/pkg/api/registry.proto
- staging/operator-registry/pkg/cache/cache_test.go
- staging/operator-registry/pkg/server/server.go
- staging/operator-registry/pkg/server/server_test.go
Comment on lines
+245
to
+249
| func (c *cache) ListPackageCustomSchemas(ctx context.Context, schema, packageName string, sender func([]byte) error) error { | ||
| mk, err := newValidatedMetaKey(schema, packageName) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid custom schema query: %v", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and its line count
wc -l staging/operator-registry/pkg/cache/cache.goRepository: openshift/operator-framework-olm
Length of output: 125
🏁 Script executed:
# Examine lines around 245-249
sed -n '245,249p' staging/operator-registry/pkg/cache/cache.goRepository: openshift/operator-framework-olm
Length of output: 334
🏁 Script executed:
# Examine lines around 310-316
sed -n '310,316p' staging/operator-registry/pkg/cache/cache.goRepository: openshift/operator-framework-olm
Length of output: 357
🏁 Script executed:
# Check for all fmt.Errorf usages with %v in this file to understand the pattern
rg 'fmt\.Errorf\(.*%v' staging/operator-registry/pkg/cache/cache.go -A 1 -nRepository: openshift/operator-framework-olm
Length of output: 1395
Use %w instead of %v for error wrapping in new error paths.
Lines 248, 312, and 315 use %v with errors, which prevents callers from inspecting root causes with errors.Is() and errors.As(). Replace with %w to preserve error chains as required by the coding guidelines.
- Line 248:
"invalid custom schema query: %v"→ use%w - Line 312:
"invalid custom schema meta: %v"→ use%w - Line 315:
"store custom schema meta %v: %v"→ use%wfor the error (second%v)
🤖 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 `@staging/operator-registry/pkg/cache/cache.go` around lines 245 - 249, Replace
the fmt.Errorf uses that currently use %v for error values with %w so the error
chain is preserved; specifically update the error construction in
ListPackageCustomSchemas (change "invalid custom schema query: %v" to use %w),
and the other two error returns that read "invalid custom schema meta: %v" and
"store custom schema meta %v: %v" so the final %v for the underlying error
becomes %w, ensuring callers can use errors.Is()/errors.As() to inspect root
causes.
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.
Summary
Cherry-pick of perdasilva/operator-registry@2dd62b3 into
staging/operator-registryand vendor.ListPackageCustomSchemasstreaming gRPC endpoint to opm serve for retrieving raw FBC blobs of non-standard (custom) schemas, scoped by packageschemaandpackageNameare required parameters; results stream asgoogle.protobuf.Structolm.package,olm.channel,olm.bundle,olm.deprecations) are excluded — they are already served by existing typed RPCs(schema, packageName), backed by both pogreb and JSON cache backendsTest plan
cache_test.go)server_test.go)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores