OPRUN-4625: Synchronize From Upstream Repositories#738
Open
openshift-bot wants to merge 105 commits into
Open
Conversation
Bumps [actions/stale](https://github.com/actions/stale) from 10 to 10.2.0. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@v10...v10.2.0) --- updated-dependencies: - dependency-name: actions/stale dependency-version: 10.2.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/containerd/containerd](https://github.com/containerd/containerd) from 1.7.31 to 1.7.32. - [Release notes](https://github.com/containerd/containerd/releases) - [Changelog](https://github.com/containerd/containerd/blob/main/RELEASES.md) - [Commits](containerd/containerd@v1.7.31...v1.7.32) --- updated-dependencies: - dependency-name: github.com/containerd/containerd dependency-version: 1.7.32 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [docker/login-action](https://github.com/docker/login-action) from 4 to 4.1.0. - [Release notes](https://github.com/docker/login-action/releases) - [Commits](docker/login-action@v4...v4.1.0) --- updated-dependencies: - dependency-name: docker/login-action dependency-version: 4.1.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.44.0 to 0.45.0. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.44.0...v0.45.0) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-version: 0.45.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [requests](https://github.com/psf/requests) from 2.33.1 to 2.34.0. - [Release notes](https://github.com/psf/requests/releases) - [Changelog](https://github.com/psf/requests/blob/main/HISTORY.md) - [Commits](psf/requests@v2.33.1...v2.34.0) --- updated-dependencies: - dependency-name: requests dependency-version: 2.34.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [regex](https://github.com/mrabarnett/mrab-regex) from 2026.4.4 to 2026.5.9. - [Changelog](https://github.com/mrabarnett/mrab-regex/blob/hg/changelog.txt) - [Commits](mrabarnett/mrab-regex@2026.4.4...2026.5.9) --- updated-dependencies: - dependency-name: regex dependency-version: 2026.5.9 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* initial vibe * integration with existing http service endpoint Signed-off-by: grokspawn <jordan@nimblewidget.com> * allow POST method only for graphql handler * functional, caching dynamic graphql server * claude-based storage/service interface division, missing preconditions * caehe on unpack; fix image verification policy Signed-off-by: grokspawn <jordan@nimblewidget.com> * schema-agnostic approaches with working tests Signed-off-by: Jordan <jordan@nimblewidget.com> * asciicast demo Signed-off-by: Jordan <jordan@nimblewidget.com> * review updates Signed-off-by: grokspawn <jordan@nimblewidget.com> --------- Signed-off-by: grokspawn <jordan@nimblewidget.com> Signed-off-by: Jordan <jordan@nimblewidget.com>
openshift-bot
added
tide/merge-method-merge
openshift-ci-robot
added
the
jira/valid-reference
|
@openshift-bot: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds dynamic per-catalog GraphQL query support: schema discovery from metas, dynamic schema build/execution, per-catalog caching with singleflight, HTTP handlers, storage integration (pre-warm/invalidate), extensive tests, docs, demo server and script, and manifest/dependency updates. ChangesGraphQL Query Feature Implementation
Demo Server and Interactive Example
User Documentation and Sample Queries
Configuration and Dependency Updates
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (1)
hack/demo/graphql-demo-script.sh (1)
52-55: ⚡ Quick winAvoid blind startup wait and hidden server errors.
Lines 52-55 can produce flaky behavior: stderr is discarded and
sleep 1assumes startup timing. Poll the endpoint with a timeout and keep stderr visible (or redirect to a temp log).Proposed fix
-"${TMPBIN}" 2>/dev/null & +"${TMPBIN}" & SERVER_PID=$! -sleep 1 +for _ in {1..20}; do + if curl -s -o /dev/null -X POST "${BASE}" \ + -H "Content-Type: application/json" \ + -d '{"query":"{ summary { totalSchemas } }"}'; then + break + fi + sleep 0.5 +done echo "Server ready. Catalog loaded: example-catalog (5 packages, 11 bundles)"🤖 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 `@hack/demo/graphql-demo-script.sh` around lines 52 - 55, Replace the blind background start that discards stderr and uses sleep (the block that launches "${TMPBIN}" and sets SERVER_PID) with a startup loop that captures stderr to a temp log (e.g., TMPLOG) instead of redirecting to /dev/null, and polls the server endpoint (via curl or wget) with a configurable timeout and backoff until it returns healthy; if the timeout elapses, kill SERVER_PID and print the temp log so failures are visible. Ensure you still export or record SERVER_PID for later cleanup and that the readiness check targets the same endpoint your script assumes is ready (replace the static sleep + echo with the polling logic and log handling).
🤖 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 `@docs/draft/howto/catalog-queries-graphql-endpoint.md`:
- Around line 28-30: The fenced code block showing the GraphQL endpoint URL is
missing a language identifier which triggers MD040; update the fence around the
URL snippet (the triple-backtick block containing
"https://localhost:8443/catalogs/<catalog-name>/api/v1/graphql") to include a
language tag such as "text" (i.e., change the opening ``` to ```text) so the
markdown linter treats it correctly.
In `@hack/demo/graphql-demo-script.sh`:
- Around line 29-34: The gql() function currently interpolates the raw query
into JSON (variable query), which breaks on quotes/newlines; fix it by building
the JSON with jq to safely escape the query: replace the inline -d "{\"query\":
\"$query\"}" with a safely generated payload like jq -n --arg q "$query"
'{"query":$q}' and pass that output to curl (e.g., curl ... --data-binary `@-` or
-d `@-`) so the GraphQL query is always validly encoded.
In `@internal/catalogd/graphql/graphql.go`:
- Around line 537-542: The sanitization that builds fieldName (using
alphanumericOnlyRE.ReplaceAllString + strings.ToLower + "s") can produce
collisions when multiple catalogSchema.Schemas map to the same fieldName, and
the current map fieldNameToSchema silently overwrites earlier entries; update
the code that iterates catalogSchema.Schemas (the block that creates fieldName
and assigns into fieldNameToSchema) to detect collisions: after computing
fieldName, check if fieldNameToSchema[fieldName] already exists and if so return
or propagate a clear error (or panic/log and abort) indicating the two
conflicting schema names and the colliding fieldName; apply the same collision
check in the second similar mapping block (the other place that derives query
field names) so collisions are rejected rather than overwritten.
In `@internal/catalogd/graphql/README.md`:
- Around line 11-13: The README code fence containing the endpoint path
`/catalogs/{catalog}/api/v1/graphql` lacks a language specifier which triggers
MD040; update the fenced code block in internal/catalogd/graphql/README.md to
use a language token (e.g., add "text" after the opening ``` so the block
becomes ```text ... ```) so the path is rendered with a specified fence
language.
In `@internal/catalogd/graphql/sample-queries.txt`:
- Line 82: Replace the incorrect root field name "bundles" with the
schema-derived root field "olmbundles" in the sample GraphQL queries (e.g.,
change the occurrences of the query fragment "bundles(limit: 5) {" to
"olmbundles(limit: 5) {"), and update all other instances where "bundles(...)"
appears in this file so examples match the generated schema (occurrences noted
around the current examples such as the ones near the shown fragment and the
other instances referenced).
In `@internal/catalogd/server/handlers.go`:
- Around line 211-214: The current JSON parsing uses
json.NewDecoder(r.Body).Decode(¶ms) and accepts the first JSON value but
doesn't verify there is no trailing data; after decoding into params, re-use the
same json.Decoder (created from r.Body) and attempt a second Decode into an
empty value (or check decoder.More()/expect io.EOF) and treat any non-EOF result
as a bad request; update the error handling around the decoder call that
currently reads json.NewDecoder(r.Body).Decode(¶ms) so it returns
http.StatusBadRequest when extra JSON is present.
In `@internal/catalogd/storage/localdir.go`:
- Around line 159-165: The current rollback removes the persisted catalogDir
when s.graphqlSvc.GetSchema(catalog, catalogFS) fails; instead stop deleting
stored content—remove the os.RemoveAll(catalogDir) rollback path and any
conditional that wraps it, and simply return the schema build error (e.g.,
return fmt.Errorf("failed to pre-build GraphQL schema for catalog %q: %w",
catalog, err)) so pre-warm failures do not destroy the persisted catalog; update
any accompanying log/error message to reflect this non-destructive behavior and
keep the call to s.graphqlSvc.GetSchema as-is.
---
Nitpick comments:
In `@hack/demo/graphql-demo-script.sh`:
- Around line 52-55: Replace the blind background start that discards stderr and
uses sleep (the block that launches "${TMPBIN}" and sets SERVER_PID) with a
startup loop that captures stderr to a temp log (e.g., TMPLOG) instead of
redirecting to /dev/null, and polls the server endpoint (via curl or wget) with
a configurable timeout and backoff until it returns healthy; if the timeout
elapses, kill SERVER_PID and print the temp log so failures are visible. Ensure
you still export or record SERVER_PID for later cleanup and that the readiness
check targets the same endpoint your script assumes is ready (replace the static
sleep + echo with the polling logic and log handling).
🪄 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: 927a9efd-63de-4e88-8fa4-c1ee1e2d9f7d
⛔ Files ignored due to path filters (130)
go.sumis excluded by!**/*.sumopenshift/tests-extension/go.sumis excluded by!**/*.sumopenshift/tests-extension/vendor/golang.org/x/net/http2/README.mdis excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/client_conn_pool.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/clientconn.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/config.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/hpack/tables.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/http2.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/server.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/server_common.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/server_wrap.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/transport.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/transport_common.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/transport_wrap.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_common.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_random.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/go118.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/idna.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/idna9.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/pre_go118.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/punycode.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables10.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables11.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables12.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables13.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables15.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables17.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables9.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/trie12.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/trie13.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/tools/go/ast/edge/edge.gois excluded by!**/vendor/**openshift/tests-extension/vendor/modules.txtis excluded by!**/vendor/**vendor/github.com/containerd/containerd/mount/temp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/containerd/containerd/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/definition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/directives.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/executor.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/extensions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/formatted.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/located.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/sortutil.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/syntax.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/graphql.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/introspection.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/kitchen-sink.graphqlis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/arguments.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/definitions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/directives.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/document.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/location.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/name.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/node.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/selections.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/type_definitions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/values.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/kinds/kinds.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/lexer/lexer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/location/location.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/parser/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/printer/printer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/source/source.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/typeInfo/type_info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/visitor/visitor.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/located.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/rules.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/rules_overlapping_fields_can_be_merged.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/scalars.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/schema-all-descriptions.graphqlis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/schema-kitchen-sink.graphqlis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/schema.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/subscription.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/type_info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/validator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/values.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pbkdf2/pbkdf2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/client_conn_pool.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/clientconn.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/hpack/tables.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/server_common.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/server_wrap.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/transport_common.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/transport_wrap.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_common.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_random.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/go118.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/idna.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/idna9.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/pre_go118.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/punycode.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables10.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables11.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables12.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables13.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables15.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables17.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables9.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/trie12.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/trie13.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ast/edge/edge.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/packages/golist.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/packages/packages.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/types/objectpath/objectpath.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gcimporter/ureader.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gocommand/version.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/testenv/testenv.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (24)
cmd/catalogd/main.godocs/draft/howto/catalog-queries-graphql-endpoint.mdgo.modhack/demo/graphql-demo-script.shhack/demo/graphql-demo-server/main.gohelm/experimental.yamlinternal/catalogd/features/features.gointernal/catalogd/graphql/README.mdinternal/catalogd/graphql/discovery_test.gointernal/catalogd/graphql/graphql.gointernal/catalogd/graphql/graphql_test.gointernal/catalogd/graphql/sample-queries.txtinternal/catalogd/server/handlers.gointernal/catalogd/server/handlers_test.gointernal/catalogd/server/http_preconditions_check.gointernal/catalogd/service/graphql_service.gointernal/catalogd/service/graphql_service_test.gointernal/catalogd/storage/localdir.gointernal/catalogd/storage/localdir_test.gointernal/shared/util/image/pull_test.gomanifests/experimental-e2e.yamlmanifests/experimental.yamlopenshift/tests-extension/go.modrequirements.txt
Comment on lines
+28
to
+30
| ``` | ||
| https://localhost:8443/catalogs/<catalog-name>/api/v1/graphql | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced endpoint code block.
This fence is missing a language tag and will trip markdown lint (MD040).
Suggested fix
-```
+```text
https://localhost:8443/catalogs/<catalog-name>/api/v1/graphql</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/draft/howto/catalog-queries-graphql-endpoint.md around lines 28 - 30,
The fenced code block showing the GraphQL endpoint URL is missing a language
identifier which triggers MD040; update the fence around the URL snippet (the
triple-backtick block containing
"https://localhost:8443/catalogs//api/v1/graphql") to include a
language tag such as "text" (i.e., change the opening totext) so the
markdown linter treats it correctly.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated reply by CodeRabbit -->
Contributor
There was a problem hiding this comment.
This is a nit. If we consider it actionable, it will be during the phase where we promote docs from the draft phase.
There was a problem hiding this comment.
@grokspawn, understood — happy to defer this until the doc graduates from draft. Would you like me to open a follow-up issue to track this (and any other lint/polish items) for when the docs are promoted?
Comment on lines
+29
to
+34
| gql() { | ||
| local query="$1" | ||
| curl -s -X POST "${BASE}" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "{\"query\": \"$query\"}" | jq . | ||
| } |
There was a problem hiding this comment.
Build GraphQL request JSON safely in gql().
Line 33 directly interpolates query into JSON, which breaks for quotes/newlines and can send invalid payloads.
Proposed fix
gql() {
local query="$1"
- curl -s -X POST "${BASE}" \
+ jq -n --arg query "$query" '{query: $query}' \
+ | curl -s -X POST "${BASE}" \
-H "Content-Type: application/json" \
- -d "{\"query\": \"$query\"}" | jq .
+ --data-binary `@-` | jq .
}📝 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
| gql() { | |
| local query="$1" | |
| curl -s -X POST "${BASE}" \ | |
| -H "Content-Type: application/json" \ | |
| -d "{\"query\": \"$query\"}" | jq . | |
| } | |
| gql() { | |
| local query="$1" | |
| jq -n --arg query "$query" '{query: $query}' \ | |
| | curl -s -X POST "${BASE}" \ | |
| -H "Content-Type: application/json" \ | |
| --data-binary `@-` | jq . | |
| } |
🤖 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 `@hack/demo/graphql-demo-script.sh` around lines 29 - 34, The gql() function
currently interpolates the raw query into JSON (variable query), which breaks on
quotes/newlines; fix it by building the JSON with jq to safely escape the query:
replace the inline -d "{\"query\": \"$query\"}" with a safely generated payload
like jq -n --arg q "$query" '{"query":$q}' and pass that output to curl (e.g.,
curl ... --data-binary `@-` or -d `@-`) so the GraphQL query is always validly
encoded.
Contributor
There was a problem hiding this comment.
This works sufficiently well to generate the demo.
There was a problem hiding this comment.
@grokspawn, understood — for a demo script with controlled query strings, the current inline interpolation is fine. Happy to leave it as-is.
Comment on lines
+537
to
+542
| fieldNameToSchema := make(map[string]string) | ||
| for schemaName := range catalogSchema.Schemas { | ||
| sanitized := alphanumericOnlyRE.ReplaceAllString(schemaName, "") | ||
| fieldName := strings.ToLower(sanitized) + "s" // e.g., "olmbundles", "olmpackages" | ||
| fieldNameToSchema[fieldName] = schemaName | ||
| } |
There was a problem hiding this comment.
Detect and reject sanitized root-field name collisions.
Line [540] and Line [556] derive query field names by stripping non-alphanumerics and appending s. Different schemas can collapse to the same field key, and the later one silently overwrites the earlier mapping.
Suggested fix
@@
fieldNameToSchema := make(map[string]string)
for schemaName := range catalogSchema.Schemas {
sanitized := alphanumericOnlyRE.ReplaceAllString(schemaName, "")
fieldName := strings.ToLower(sanitized) + "s" // e.g., "olmbundles", "olmpackages"
- fieldNameToSchema[fieldName] = schemaName
+ if existing, exists := fieldNameToSchema[fieldName]; exists && existing != schemaName {
+ return nil, fmt.Errorf("schema field name collision: %q and %q both map to %q", existing, schemaName, fieldName)
+ }
+ fieldNameToSchema[fieldName] = schemaName
}Also applies to: 555-559
🤖 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 `@internal/catalogd/graphql/graphql.go` around lines 537 - 542, The
sanitization that builds fieldName (using alphanumericOnlyRE.ReplaceAllString +
strings.ToLower + "s") can produce collisions when multiple
catalogSchema.Schemas map to the same fieldName, and the current map
fieldNameToSchema silently overwrites earlier entries; update the code that
iterates catalogSchema.Schemas (the block that creates fieldName and assigns
into fieldNameToSchema) to detect collisions: after computing fieldName, check
if fieldNameToSchema[fieldName] already exists and if so return or propagate a
clear error (or panic/log and abort) indicating the two conflicting schema names
and the colliding fieldName; apply the same collision check in the second
similar mapping block (the other place that derives query field names) so
collisions are rejected rather than overwritten.
Contributor
There was a problem hiding this comment.
Both sections literally contain tests to see if the derived fieldname results in collision. Disregard.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the full content around the fieldNameToSchema blocks
rg -n "fieldNameToSchema|fieldName collision|collision|existing" internal/catalogd/graphql/graphql.go -A2 -B2Length of output: 2173
🏁 Script executed:
#!/bin/bash
# Show broader context around the fieldNameToSchema and root query field building
sed -n '535,650p' internal/catalogd/graphql/graphql.goLength of output: 3919
@grokspawn, I checked the actual code and unfortunately the collision detection doesn't appear to be there. Both blocks silently overwrite on duplicate keys:
Block 1 (lines 537–542):
fieldNameToSchema := make(map[string]string)
for schemaName := range catalogSchema.Schemas {
sanitized := alphanumericOnlyRE.ReplaceAllString(schemaName, "")
fieldName := strings.ToLower(sanitized) + "s"
fieldNameToSchema[fieldName] = schemaName // silent overwrite
}Block 2 (~lines 548–558):
queryFields[fieldName] = &graphql.Field{ ... } // also silent overwriteNeither block checks whether fieldName is already present before assigning. If two schema names (e.g. olm.bundle and olmbundle) strip to the same key (olmbundles), the second one quietly replaces the first in both maps, leading to the later schema's resolver being used for both field names — and the first schema becoming unreachable via GraphQL.
Would you like me to open a follow-up issue to track this, or would you prefer to address it directly in this PR?
Comment on lines
+11
to
+13
| ``` | ||
| /catalogs/{catalog}/api/v1/graphql | ||
| ``` |
There was a problem hiding this comment.
Specify the fenced code language for the endpoint path block.
Missing code-fence language can fail markdown lint (MD040).
Suggested fix
-```
+```text
/catalogs/{catalog}/api/v1/graphql</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @internal/catalogd/graphql/README.md around lines 11 - 13, The README code
fence containing the endpoint path /catalogs/{catalog}/api/v1/graphql lacks a
language specifier which triggers MD040; update the fenced code block in
internal/catalogd/graphql/README.md to use a language token (e.g., add "text"
after the opening so the block becomestext ... ```) so the path is
rendered with a specified fence language.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
|
||
| # Get bundles with all properties | ||
| { | ||
| bundles(limit: 5) { |
There was a problem hiding this comment.
Use the actual schema-derived root field name (olmbundles) in these examples.
These samples use bundles(...), but the documented/generated field naming pattern uses olmbundles. As written, these queries are likely to fail for users.
Suggested fix
- bundles(limit: 5) {
+ olmbundles(limit: 5) {
...
- bundles(limit: 20) {
+ olmbundles(limit: 20) {
...
- bundles(limit: 10) {
+ olmbundles(limit: 10) {
...
- bundles(limit: 10) {
+ olmbundles(limit: 10) {Also applies to: 121-121, 145-145, 163-163
🤖 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 `@internal/catalogd/graphql/sample-queries.txt` at line 82, Replace the
incorrect root field name "bundles" with the schema-derived root field
"olmbundles" in the sample GraphQL queries (e.g., change the occurrences of the
query fragment "bundles(limit: 5) {" to "olmbundles(limit: 5) {"), and update
all other instances where "bundles(...)" appears in this file so examples match
the generated schema (occurrences noted around the current examples such as the
ones near the shown fragment and the other instances referenced).
Contributor
There was a problem hiding this comment.
dueling agents didn't find these. I'll rank them as nits for now, and try to keep them in mind for follow-ups.
Alternatively, you can open an issue for this nit in github.com/operator-framework/operator-controller.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo operator-framework/operator-controller \
--title "fix: sample-queries.txt uses incorrect root field name 'bundles' instead of 'olmbundles'" \
--body "## Summary
The file \`internal/catalogd/graphql/sample-queries.txt\` contains several GraphQL query examples that reference the root field name \`bundles\`, but the dynamic schema generator derives the field name from the schema name (\`olm.bundle\` → \`olmbundles\`). As written, those sample queries will fail for users.
## Affected lines (approx.)
- Line 82: \`bundles(limit: 5) {\`
- Line 121: \`bundles(limit: 20) {\`
- Line 145: \`bundles(limit: 10) {\`
- Line 163: \`bundles(limit: 10) {\`
## Suggested fix
Replace all occurrences of \`bundles(\` with \`olmbundles(\` in \`internal/catalogd/graphql/sample-queries.txt\`.
## References
- Downstream PR that introduced this file: https://github.com/openshift/operator-framework-operator-controller/pull/738
- Review comment that surfaced this issue: https://github.com/openshift/operator-framework-operator-controller/pull/738#discussion_r3307700386
Reported by `@grokspawn`."Length of output: 1233
@grokspawn, done! The upstream issue has been opened here: operator-framework/operator-controller#2726
Comment on lines
+211
to
+214
| if err := json.NewDecoder(r.Body).Decode(¶ms); err != nil { | ||
| http.Error(w, "Invalid request body", http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
Reject trailing JSON payload after the GraphQL request object.
Line [211] accepts the first JSON object but does not verify EOF. A body with extra trailing JSON currently passes parsing.
Suggested fix
- if err := json.NewDecoder(r.Body).Decode(¶ms); err != nil {
+ dec := json.NewDecoder(r.Body)
+ dec.DisallowUnknownFields()
+ if err := dec.Decode(¶ms); err != nil {
http.Error(w, "Invalid request body", http.StatusBadRequest)
return
}
+ if err := dec.Decode(&struct{}{}); !errors.Is(err, io.EOF) {
+ http.Error(w, "Invalid request body", http.StatusBadRequest)
+ return
+ }📝 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
| if err := json.NewDecoder(r.Body).Decode(¶ms); err != nil { | |
| http.Error(w, "Invalid request body", http.StatusBadRequest) | |
| return | |
| } | |
| dec := json.NewDecoder(r.Body) | |
| dec.DisallowUnknownFields() | |
| if err := dec.Decode(¶ms); err != nil { | |
| http.Error(w, "Invalid request body", http.StatusBadRequest) | |
| return | |
| } | |
| if err := dec.Decode(&struct{}{}); !errors.Is(err, io.EOF) { | |
| http.Error(w, "Invalid request body", http.StatusBadRequest) | |
| return | |
| } |
🤖 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 `@internal/catalogd/server/handlers.go` around lines 211 - 214, The current
JSON parsing uses json.NewDecoder(r.Body).Decode(¶ms) and accepts the first
JSON value but doesn't verify there is no trailing data; after decoding into
params, re-use the same json.Decoder (created from r.Body) and attempt a second
Decode into an empty value (or check decoder.More()/expect io.EOF) and treat any
non-EOF result as a bad request; update the error handling around the decoder
call that currently reads json.NewDecoder(r.Body).Decode(¶ms) so it returns
http.StatusBadRequest when extra JSON is present.
Contributor
There was a problem hiding this comment.
I'm not sure that this is a reasonable, realistic scenario while technically valid. If this were a human, I'd say to open an issue in github.com/operator-framework/operator-controller and that this shouldn't impede merging this PR.
There was a problem hiding this comment.
@grokspawn, that's a fair call — the scenario is contrived and doesn't warrant blocking this sync PR.
Would you like me to open a follow-up issue in operator-framework/operator-controller to track the trailing-JSON / EOF validation hardening in handleV1GraphQL? If so, I can create it now.
Comment on lines
+159
to
+165
| if _, err := s.graphqlSvc.GetSchema(catalog, catalogFS); err != nil { | ||
| // Schema build failed - rollback by removing the catalog directory | ||
| // to maintain consistency (don't persist catalog without valid schema) | ||
| if removeErr := os.RemoveAll(catalogDir); removeErr != nil { | ||
| return fmt.Errorf("failed to pre-build GraphQL schema for catalog %q: %w (rollback also failed: %v)", catalog, err, removeErr) | ||
| } | ||
| return fmt.Errorf("failed to pre-build GraphQL schema for catalog %q: %w", catalog, err) |
There was a problem hiding this comment.
Avoid deleting the stored catalog when GraphQL pre-warm fails.
At Line 160, a schema pre-warm error removes catalogDir after the swap at Line 144–Line 147. That turns cache-build failures into catalog unavailability. Keep persisted content non-destructive and fail pre-warm separately.
💡 Proposed fix
if _, err := s.graphqlSvc.GetSchema(catalog, catalogFS); err != nil {
- // Schema build failed - rollback by removing the catalog directory
- // to maintain consistency (don't persist catalog without valid schema)
- if removeErr := os.RemoveAll(catalogDir); removeErr != nil {
- return fmt.Errorf("failed to pre-build GraphQL schema for catalog %q: %w (rollback also failed: %v)", catalog, err, removeErr)
- }
+ // Keep stored catalog content intact; only cache pre-warm failed.
+ s.graphqlSvc.InvalidateCache(catalog)
+ klog.ErrorS(err, "failed to pre-build GraphQL schema cache", "catalog", catalog)
return fmt.Errorf("failed to pre-build GraphQL schema for catalog %q: %w", catalog, err)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/catalogd/storage/localdir.go` around lines 159 - 165, The current
rollback removes the persisted catalogDir when s.graphqlSvc.GetSchema(catalog,
catalogFS) fails; instead stop deleting stored content—remove the
os.RemoveAll(catalogDir) rollback path and any conditional that wraps it, and
simply return the schema build error (e.g., return fmt.Errorf("failed to
pre-build GraphQL schema for catalog %q: %w", catalog, err)) so pre-warm
failures do not destroy the persisted catalog; update any accompanying log/error
message to reflect this non-destructive behavior and keep the call to
s.graphqlSvc.GetSchema as-is.
Contributor
There was a problem hiding this comment.
This is the desired outcome -- at least for now. Failure to derive a graphql schema either means that the catalog is poorly-formed FBC (and likely would've been flagged/failed at an earlier point) so the action is warranted, OR that the FBC schemas include new variations which reduce to the same graphql schema representations, in which case the reduced functionality still violates the principle of least-surprise.
Discarding this input.
There was a problem hiding this comment.
@grokspawn, understood — the rollback-on-schema-failure is intentional by design. I'll keep that in mind for future reviews.
✏️ Learnings added
Learnt from: grokspawn
Repo: openshift/operator-framework-operator-controller PR: 738
File: internal/catalogd/storage/localdir.go:159-165
Timestamp: 2026-05-27T20:28:53.092Z
Learning: In `internal/catalogd/storage/localdir.go`, the `Store()` method deliberately removes the catalog directory (`os.RemoveAll(catalogDir)`) when the GraphQL schema pre-warm (`s.graphqlSvc.GetSchema`) fails. This rollback is intentional: a schema build failure signals either (1) malformed FBC that would have been caught earlier, or (2) new FBC schema variations that reduce to ambiguous/conflicting GraphQL representations — both cases warrant removing the persisted catalog to avoid violating the principle of least-surprise for consumers. Do not flag this rollback as a bug or suggest making it non-destructive.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
|
/test openshift-e2e-aws |
Contributor
|
@grokspawn the AI did some reviews regards the change: operator-framework/operator-controller@1416535 You might want to give a look . Thanks 🙌 |
Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: Update generate-manifests to handle new directory The `default` directory was renamed `base`. Signed-off-by: Todd Short <todd.short@me.com> The `base` directory was moved to `base\operator-controller`. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Drop commitchecker Signed-off-by: Alexander Greene <greene.al1991@gmail.com> UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART Reconciling with https://github.com/openshift/ocp-build-data/tree/4022cd290f00a44d667dda03f2d78d84a488c7ed/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: update owners * Remove alumni from owners * Add m1kola to approvers Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: Add pointer to tooling README UPSTREAM: <carry>: Disable Validating Admission Policy APIs downstream Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.16 Reconciling with https://github.com/openshift/ocp-build-data/tree/6250d54c4686a708ca5985afb73080e8ca9a1f7f/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: Enable Validating Admission Policy APIs downstream * This reverts commit 3f079c4. * Includes Validating Admission Policy manifests Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: manifests: set required-scc for openshift workloads UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.17 Reconciling with https://github.com/openshift/ocp-build-data/tree/4c1326094222f9209876f06833179a1b9178faf7/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: add everettraven to approvers+reviewers Signed-off-by: everettraven <everettraven@gmail.com> UPSTREAM: <carry>: add openshift kustomize overlay to enable TLS communication with catalogd. Configure the CA certs using the configmap injection method via service-ca-operator Signed-off-by: everettraven <everettraven@gmail.com> UPSTREAM: <carry>: Add tmshort to approvers Also `s/runtime/framework/g` in the DOWNSTREAM_OWNERS Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.18 Reconciling with https://github.com/openshift/ocp-build-data/tree/dd68246f3237db5db458127566fc7b05b55e1660/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: Properly copy and call kustomize Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: manifests: add hostPath mount for /etc/containers Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Add test-e2e target for downstream Makefile to be run by openshift/release. Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: Add downstream verify makefile target Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: openshift: template log verbosity to be managed by cluster-olm-operator Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Add global-pull-secret flag Pass global-pull-secret to the manager container. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: Update openshift CAs to operator-controller The /run/secrets/kubernetes.io/serviceaccount/ directory is projected into the pod and contains the following CA certificates: * configmap/kube-root-ca.crt as ca.crt * configmap/openshift-service-ca.crt as service-ca.crt Update the --ca-certs-dir argument to reference the directory. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add HowTo for origin tests Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add e2e registry Dockerfile Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: add nodeSelector and tolerations to operator-controller deployment via kustomize patch Signed-off-by: everettraven <everettraven@gmail.com> UPSTREAM: <carry>: namespace: use privileged PSA for audit and warn levels Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Enable downstream e2e Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: Remove m1kola from owners Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.19 Reconciling with https://github.com/openshift/ocp-build-data/tree/a39508c86497b4e5e463d7b2c78e51e577be9e7d/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: generate and mount service-ca server cert Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Add support for proxy trustedCAs Just map the list of trusted ca certs into the deployment Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Fix error to build the image Copy correct (new) executable name for operator-controller Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Fix make verify for mac os envs Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Move operator-controller openshift files to its own dir UPSTREAM: <carry>: Upgrade OCP images from 4.18 to 4.19 UPSTREAM: <carry>: Add Openshift's catalogd manifests - Move to openshift/catalogd the specific manifest under: https://github.com/openshift/operator-framework-catalogd/tree/main/openshift - Add call to generate catalogd manifest to 'make manifest'. Make verify test is now done for catalogd and operator-controller Openshift's manifests UPSTREAM: <carry>: resolve issue with pre-mature mounting of trusted CA configmap Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Add /etc/docker to the operator-controller and catalogd deployments This allows for use of the any image.config.openshift.io trusted CAs Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: fixup catalogd.Dockerfile paths Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Resolve issue with pre-mature mounting of service CA configmap Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: use projected volume for CAs to avoid subPath limitations Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Revert "UPSTREAM: <carry>: use projected volume for CAs to avoid subPath limitations" This reverts commit 548caa4. UPSTREAM: <carry>: use projected volume for CAs to avoid subPath limitations Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Remove vet from openshift verify The `vet` target was removed upstream. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Skip another upstream test Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Cleanup openshift/Makefile by removing no longer required comments regards catalogd e2e tests UPSTREAM: <carry>: Enable OCP metrics collection by default Enables OCP to collect Prometheus metrics for both catalogd and operator-controller by default. This is accomplished via ServiceMonitor CRs which are now created for both projects. UPSTREAM: <carry>: Fix catalogd.Dockerfile to use new paths The root catalogd directory has been removed Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Update DOWNSTREAM_OWNERS_ALIASES Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add openshift node selector annotation Signed-off-by: Catherine Chan-Tse <cchantse@redhat.com> (cherry picked from commit 9b4a113) UPSTREAM: <carry>: Add caalogd-cas-dir option to op-con Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: set the SElinux type Signed-off-by: Jian Zhang <jiazha@redhat.com> UPSTREAM: <carry>: Add initial stack to run tests to validate the catalogs UPSTREAM: <carry>: Add vendor files for the catalog-sync tests UPSTREAM: <carry>: Bump catalog versions to 4.19 Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: revert "Bump catalog versions to 4.19" This reverts commit a98980b. UPSTREAM: <carry>: Update HOWTO-origin-tests techpreview is no longer a required option. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [DefaultCatalogTests]: Allow to pass auth path for docker credentials" UPSTREAM: <carry>: fix: set NoLchown=true to allow image unpack on OCPci UPSTREAM: <carry>: [DefaultCatalogTests]: Moving parse of ENVVAR to the caller (follow-up 345) UPSTREAM: <carry>: [Default Catalog]: Create tmp dir to extract layers with right permissions to avoid issues scenarios UPSTREAM: <carry>: [Default Catalog](cleanp) Remove hack directory which is not used UPSTREAM: <carry>: Change code implementation to extract layers in OCP env UPSTREAM: <carry>: Add vendor files for change in the extract code implementation UPSTREAM: <carry>: [Default Catalog Tests]: Final cleanups and enhancements of initial implementation UPSTREAM: <carry>: SELinux type for operator-controller Signed-off-by: Jian Zhang <jiazha@redhat.com> UPSTREAM: <carry>: Bump catalog versions to 4.19 Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [Default Catalog Consistency Test] (feat) add check for executable files in filesystem Checks if given paths exist and point to executable files or valid symlinks. UPSTREAM: <carry>: [Default Catalog Consistency Test]: fix junit output format to allow generate xml UPSTREAM: <carry>: [Default Catalog Consistency Test] (feat) add check to validate multi-arch support UPSTREAM: <carry>: [Default Catalog Consistency Test]: Enable CatalogChecks UPSTREAM: <carry>: [Default Catalog Consistency Test]: Rename Tests suite and small cleanups UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.20 Reconciling with https://github.com/openshift/ocp-build-data/tree/dfb5c7d531490cfdc61a3b88bc533702b9624997/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: Updating ose-olm-catalogd-container image to be consistent with ART for 4.20 Reconciling with https://github.com/openshift/ocp-build-data/tree/dfb5c7d531490cfdc61a3b88bc533702b9624997/images/ose-olm-catalogd.yml UPSTREAM: <carry>: Update e2e registry to use 1.24/4.20 Update the e2e registry Dockerfile to use golang 1.24/OCP 4.20 Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [Catalog Default Tests]: Upgrade go version to 1.24.3, dependencies and fix new lint issue UPSTREAM: <carry>: Add structure to allow move the orgin tests using OTE This commit introduces a binary and supporting structure to enable the execution of OpenShift origin (olmv1) tests using the Open Test Environment (OTE). It lays the groundwork for moving origin test in openshift/origin to be executed from this repository using OTE. UPSTREAM: <carry>: Add support for experimental manifests Update the openshift kustomize configuration for both operator-controller and catalogd. Update the manifest generation scripts to put the core generation code into a function (ignore-whitespace will help with the review), so that it can be called twice; once for standard, and once for experimental. Move around some of the kustomization directives to * Create a patch kustomization (Component) file and move the patch directives from olmv1-ns there. This allows it to be referenced from a different directory. * Add a kustomization file for tusted-ca. This allows it to be referenced from a different directory. * Move the setting of the namePrefix for operator-controller; this makes the generation compatible with upstream feature components. * Define experimental kustomization files that reference existing components. * Reference the correct CRDs (standard or experimental). * Add references to upstream feature components into the experimental manifests. This *will* add `--feature-gates` options from the upstream feature components to the experimental manifests. The cluster-olm-operator will strip those arguments from the deployments before adding the enabled feature gates. Update the Dockerfiles to include the experimental manifests and a copy script (`cp-manifests`) into the image containers. The complexity of having multiple sets of manifests mean that the simple initContainer copy mechanism found in cluster-olm-operator is no longer sufficient. This attempts to keep backwards compatibility with older versions of cluster-olm-operator, specifically by keeping the original (standard) manifests in the original location, and adding the experimental manifests in a new directory. The new `cp-manifests` script is used by newer versions of cluster-olm-operator. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE] - chore: follow up openshift#383 – remove unreachable target call UPSTREAM: <carry>: Remove build of test image registry Upstream now uses a different image Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add test-experimental-e2e target to openshift Makefile This adds a test-experimental-e2e target to allow the CI to run the experimental e2e test. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE]: Add binary in the operator controller image to allow proper integration with OCP tests UPSTREAM: <carry>: Fix experimental manifest copying The standard manifest was being copied rather than the experimental manifest. This meant that the expected feature-flags are not present. This is failing now that we are doing a check for those feature-flags. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Update manifest generation for upstream rbac/webhooks Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE] - Add tracking mechanism UPSTREAM: <carry>: Update OTE dep to get fix UPSTREAM: <carry>: [OTE] Add Readme UPSTREAM: <carry>: set GIT_COMMIT env from SOURCE_GIT_COMMIT in Dockerfiles for operator-controller and catalogd Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com> UPSTREAM: <carry>: add openshift specific build target to pass commit info downstream Signed-off-by: Ankita Thomas <ankithom@redhat.com> UPSTREAM: <carry>: add source commit into binaries when linking - Removes extra GIT_COMMIT set - fixup Dockerfiles after rebase - consider "" unset so build-info can fill commit/date - double quote go flags & honor GIT_COMMIT if set - improve robustness of build-info parsing - Trim whitespace on all version fields - isUnset and valueOrUnknown now call strings.TrimSpace - Avoid clobbering values injected via ldflags - set repoState from build-info only when repoState is still unset - set version from build-info only when unset and build-info value is non-empty UPSTREAM: <carry>: OTE add first test from openshift/origin olmv1.go UPSTREAM: <carry>: Migrate tasks from openshift/origin olm v1.go file which are remaining This commit moves the final OLMv1 tests from openshift/origin/test/extended/olm/olmv1.go to their proper location in this repository. This migration is part of a larger effort to streamline development by co-locating tests with the component they validate. This will reduce CI overhead and allow for faster, more atomic changes. Assisted-by: Gemini UPSTREAM: <carry>: OTE - How to test locally with OCP instances UPSTREAM: <carry>: [OTE] Refac: refac helper and olmv1 test to create namespace instead to use pre-existent UPSTREAM: <carry>: [OTE] add webhook tests Migrates OLMv1 webhook operator tests from using external YAML files to defining resources in Go structs. This change removes file dependencies, improving test reliability and simplifying test setup. The migration is a refactoring of code from openshift/origin#30059. The new code uses better naming conventions and adapts the tests to work with a controller-runtime client, enhancing test consistency and maintainability. The migration covers all core test scenarios: - Validating, mutating, and conversion webhooks. - Certificate and secret rotation tolerance. Assisted-by: Gemini UPSTREAM: <carry>: OTE: rewrite the upgrade incompatible operator test This test replaces the existing upgrade incompatible test. The main change is that operator and catalog bundles are created on-the-fly to support OCP 4.20. This means we are no longer dependent on public operators for this test. This creates new bundles in the OCP ImageRegistry, this requires using a number of OCP APIs, including using a raw API URL to invoke the build. This is done by invoking an external k8s client (either `oc` or `kubectl`), and passing it a tarball of the bundle to be created. So, it can't be done by the golang k8sClient normally available (i.e. the create input is a tarball not a YAML file). This introduces the use of go-bindata to store the bundle contents. It also pulls in openshift mage, buld and operator APIs. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Handle service-ca cert availability/rotation There is problem when the service-ca certificate is not available at pod start. This is an issue because the SystemCertPool is created from SSL_CERT_DIR, which may include the empty service-ca. The SystemCertPool is never regenerated during the lifetime of the program execution, so it will never get updated when the service-ca is filled. Thus, we need to use --pull-cas-dir to reference the CAs that we want to use. This will also allow OLMv1 to reload the service-ca when it is reloaded (after 2 years, mind you). Removing the SSL_CERT_DIR setting, and adding the --pull-cas-dir flag ought to be equivalent to what we have now (i.e. SSL_CERT_DIR and no --pull-cas-dir), except that rotation will be handled better. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE] add webhook tests Revert "UPSTREAM: <carry>: [OTE] add webhook tests" This reverts commit 9963614. UPSTREAM: <carry>: Upgrade OCP Catalog images from 4.19 to 4.20 UPSTREAM: <carry>: Remove bindata generation from build Using go-bindata is causing problems with ART builds. This removes the use of go-bindata from the builds. This will subsequently require that users MANUALLY run the `bindata` target to refresh the bindata, or use the `build-update` target. This is a quickfix to put out the fire. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE] Add webhook tests - Add dumping of container logs and `kubectl describe pods` output for better diagnostics. - Include targeted certificate details dump (`tls.crt` parse) when failures occur. - Add additional check to verify webhook responsiveness after certificate rotation. This change is a refactor of code from openshift/origin#30059. Assisted-by: Gemini UPSTREAM: <carry>: OTE add logs and dumps for olmv1 test and fix helper for clusterextensions UPSTREAM: <carry>: [OTE] Migrate preflight checks from openshift/origin Migrated OLMv1 operator preflight checks from using external YAML files to defining ClusterRole permissions directly in Go structs. This improves test reliability and simplifies test setup by removing file dependencies. The changes ensure precise replication of original test scenarios, including specific permission omissions for services, create verbs, ClusterRoleBindings, ConfigMap resourceNames, and escalate/bind verbs. Assisted-by: Gemini UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation This change is a refactor of code from openshift/origin#30059. Assisted-by: Gemini UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests UPSTREAM: <carry>: remove obsolete owners Signed-off-by: grokspawn <jordan@nimblewidget.com> UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin This commit migrates the olmv1_catalog set of tests from openshift/origin to OTE as part the broad effort to migrate all tests. Assisted-by: Gemini UPSTREAM: <carry>: Migrate single/own namespace tests This commit migrates the OLMv1 single and own namespace watch mode tests from openshift/origin/test/extended/olm/olmv1-singleownnamespace.go to this repository. This is part of the effort to move component-specific tests into their respective downstream locations. Assisted-by: Gemini UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. This reverts commit 0bb1953. UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation This reverts commit e9e3220. UPSTREAM: <carry>: Ensure unique name for bad-catalog tests UPSTREAM: <carry>: Revert "Handle service-ca cert availability/rotation" This reverts commit 9cc13d8. UPSTREAM: <carry>: grant QE approver permission for OTE UPSTREAM: <carry>: Update webhook ote tests to use latest webhook-operator Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> UPSTREAM: <carry>: update operator-controller to v1.5.1 UPSTREAM: <carry>: configure watchnamespace using spec.config for OTE tests UPSTREAM: <carry>: add jiazha to approvers UPSTREAM: <carry>: Create combined manifests for comparison Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Use Helm charts for openshift manifests Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: add support for tests-private cases and add the case UPSTREAM: <carry>: Fix cp-manifests copying of helm charts The method used to copy the helm charts is including an extra `helm` directory in the destination path, that is making the cluster-olm-operator code just a bit more complicated than it needs to be. This fixes the copy location. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Remove kustomize manifests from images and repo Now that helm manifests are being used to dynamically generate the manifests, the pre-generated manifests are no longer needed. So, we can remove them from the repo and the images. However, because we still want to verify the manifests are "good", we are still creating a "single-file" version of the manifests for verification purposes, and to allow us to see what changes are happening to the manifests (from upstream and/or downstream sources). Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add pedjak and trgeiger as reviewers UPSTREAM: <carry>: migrate more cases from tests-private and enhance suites with filters UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.21 Reconciling with https://github.com/openshift/ocp-build-data/tree/4fbe3fab45239dc4be6f5d9d98a0bf36e0274ec9/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: Updating ose-olm-catalogd-container image to be consistent with ART for 4.21 Reconciling with https://github.com/openshift/ocp-build-data/tree/4fbe3fab45239dc4be6f5d9d98a0bf36e0274ec9/images/ose-olm-catalogd.yml UPSTREAM: <carry>: OTE: Enable disconnected environment and build test operator controller image Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> UPSTREAM: <carry>: for incompatible test add func to wait builder and deployer SA creation by OCP controller UPSTREAM: <carry>: Fix VERSION replacement in catalog bindata Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: check kubeconfig only run-test and run-suite UPSTREAM: <carry>: Clean up cp-manifests There is no longer a need to copy conditionally Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Update does-not-exist and simple install to work in a disconnected environment Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: support webhook case in disconnected UPSTREAM: <carry>: Consolidate build API This consolidates the in-cluster building of a bundle and catalog. The catalog and bundle bindata are inputs, along with a set of replacements so that catalog and bundle templates can be used to create the images. This can be done in the BeforeEach() for a set of tests that use the same data. Signed-off-by: Todd Short <todd.short@me.com>
…images from openshift/catalogd/manifests.yaml
Signed-off-by: Todd Short <todd.short@me.com>
…oss to avoid flakes
Signed-off-by: Todd Short <todd.short@me.com>
…uess and waiting for k8s cleanups Co-Author: kuiwang@redhat.com
Signed-off-by: Todd Short <todd.short@me.com>
…to run outside of OCP
…ffinity for HA topology Rolling updates in HighlyAvailable clusters leave catalogd and operator-controller unavailable when the only running pod is evicted before its replacement is ready. Fix by defaulting replicas=1 and PDB disabled in the static Helm values (safe for SNO/External topologies, passes the SNO conformance test that asserts exactly one replica in SingleReplica topology mode). Add pod anti-affinity to prefer scheduling replicas on different nodes. cluster-olm-operator detects the cluster's ControlPlaneTopology at startup and overrides these values to replicas=2 and PDB enabled when a HighlyAvailable topology is detected, then re-renders the manifests before starting controllers. When a topology change is observed at runtime (exceedingly rare), the operator exits so its deployment controller restarts it, triggering a fresh Helm render with the correct values for the new topology. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
…etween both-watch-modes scenarios The both-watch-modes test loops over two scenarios (singlens, ownns) inside a single It block and was blocking on full namespace deletion between them. This caused flaky 300s timeouts on GCP techpreview clusters where master nodes run at 94-99% CPU, which starves the namespace controller and makes namespace termination arbitrarily slow. The wait was not guarding anything real: - EnsureCleanupClusterExtension already ensures the CE and CRD are gone; since CE deletion uses ForegroundPropagation, the ClusterObjectSet teardown must complete before the CE disappears, meaning all managed resources (Deployments, Services, etc.) are already deleted at that point. - The singleown bundle installs no ValidatingWebhookConfiguration or MutatingWebhookConfiguration, so there is no webhook admission risk. - Each scenario generates unique namespace names and CRD group suffixes via rand.String(4), so a terminating namespace from scenario 1 cannot collide with or interfere with scenario 2's resources. Trigger both namespace deletions and proceed without waiting. The DeferCleanup registrations that already exist will handle any residual cleanup after the spec exits. Fixes: OCPBUGS-84943 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
- Replace broken test-experimental-e2e target (test/experimental-e2e no longer exists) with /bin/true so triggered jobs always succeed - Pass -timeout=60m to go test; the previous invocation relied on Go's 10m default which is too short for BoxcutterRuntime clusters - Set E2E_STEP_TIMEOUT=15m; BoxcutterRuntime applies resources through sequential phases (CRD must reach Established before the deploy phase starts), making installations slower than the upstream 5m default - Skip ~@CatalogdHA scenarios (require multiple catalogd replicas not present in standard topology) - Skip ~@ProgressDeadline scenarios (require progressDeadlineMinutes < 10 but the OpenShift CRD enforces a minimum of 10) - Skip ~@httpproxy scenarios (too disruptive to cluster networking) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
The e2e-test-registry image is no longer built by CI after openshift/release#78581 removed it from the CI config. The dynamic per-scenario catalog system replaced the pre-built registry image, making this Dockerfile dead code.
It's no longer bring used. Signed-off-by: Todd Short <tshort@redhat.com>
openshift-bot
force-pushed
the
synchronize-upstream
branch
from
f56ed94 to
4582578
Compare
Contributor
|
New changes are detected. LGTM label has been removed. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: openshift-bot 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hack/demo/graphql-demo-server/main.go`:
- Line 51: The call to url.Parse("/catalogs/") ignores the returned error;
change the assignment to capture the error (e.g., rootURL, err :=
url.Parse("/catalogs/")) and handle it immediately (fail fast) by
logging/terminating on error (e.g., log.Fatalf or similar) so that rootURL is
not used when parsing failed; update usage around the rootURL variable
accordingly.
🪄 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: 0bf09d67-8a31-430c-a733-1ba7fbdec9d4
⛔ Files ignored due to path filters (130)
go.sumis excluded by!**/*.sumopenshift/tests-extension/go.sumis excluded by!**/*.sumopenshift/tests-extension/vendor/golang.org/x/net/http2/README.mdis excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/client_conn_pool.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/clientconn.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/config.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/hpack/tables.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/http2.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/server.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/server_common.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/server_wrap.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/transport.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/transport_common.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/transport_wrap.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_common.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_random.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/go118.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/idna.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/idna9.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/pre_go118.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/punycode.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables10.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables11.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables12.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables13.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables15.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables17.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/tables9.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/trie12.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/idna/trie13.0.0.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!**/vendor/**openshift/tests-extension/vendor/golang.org/x/tools/go/ast/edge/edge.gois excluded by!**/vendor/**openshift/tests-extension/vendor/modules.txtis excluded by!**/vendor/**vendor/github.com/containerd/containerd/mount/temp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/containerd/containerd/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/definition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/directives.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/executor.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/extensions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/formatted.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/located.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/sortutil.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/gqlerrors/syntax.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/graphql.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/introspection.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/kitchen-sink.graphqlis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/arguments.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/definitions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/directives.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/document.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/location.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/name.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/node.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/selections.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/type_definitions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/ast/values.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/kinds/kinds.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/lexer/lexer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/location/location.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/parser/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/printer/printer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/source/source.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/typeInfo/type_info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/language/visitor/visitor.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/located.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/rules.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/rules_overlapping_fields_can_be_merged.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/scalars.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/schema-all-descriptions.graphqlis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/schema-kitchen-sink.graphqlis excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/schema.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/subscription.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/type_info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/validator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/graphql-go/graphql/values.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pbkdf2/pbkdf2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/client_conn_pool.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/clientconn.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/config.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/hpack/tables.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/http2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/server.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/server_common.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/server_wrap.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/transport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/transport_common.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/transport_wrap.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_common.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc7540.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_random.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/http2/writesched_roundrobin.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/go118.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/idna.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/idna9.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/pre_go118.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/punycode.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables10.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables11.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables12.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables13.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables15.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables17.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/tables9.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/trie12.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/idna/trie13.0.0.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/httpcommon/request.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/ast/edge/edge.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/packages/golist.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/packages/packages.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/go/types/objectpath/objectpath.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gcimporter/ureader.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/gocommand/version.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/tools/internal/testenv/testenv.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (24)
cmd/catalogd/main.godocs/draft/howto/catalog-queries-graphql-endpoint.mdgo.modhack/demo/graphql-demo-script.shhack/demo/graphql-demo-server/main.gohelm/experimental.yamlinternal/catalogd/features/features.gointernal/catalogd/graphql/README.mdinternal/catalogd/graphql/discovery_test.gointernal/catalogd/graphql/graphql.gointernal/catalogd/graphql/graphql_test.gointernal/catalogd/graphql/sample-queries.txtinternal/catalogd/server/handlers.gointernal/catalogd/server/handlers_test.gointernal/catalogd/server/http_preconditions_check.gointernal/catalogd/service/graphql_service.gointernal/catalogd/service/graphql_service_test.gointernal/catalogd/storage/localdir.gointernal/catalogd/storage/localdir_test.gointernal/shared/util/image/pull_test.gomanifests/experimental-e2e.yamlmanifests/experimental.yamlopenshift/tests-extension/go.modrequirements.txt
✅ Files skipped from review due to trivial changes (3)
- helm/experimental.yaml
- internal/catalogd/server/http_preconditions_check.go
- internal/catalogd/graphql/sample-queries.txt
🚧 Files skipped from review as they are similar to previous changes (17)
- requirements.txt
- manifests/experimental-e2e.yaml
- internal/shared/util/image/pull_test.go
- internal/catalogd/features/features.go
- manifests/experimental.yaml
- cmd/catalogd/main.go
- internal/catalogd/graphql/graphql_test.go
- go.mod
- hack/demo/graphql-demo-script.sh
- internal/catalogd/service/graphql_service_test.go
- openshift/tests-extension/go.mod
- internal/catalogd/graphql/discovery_test.go
- internal/catalogd/service/graphql_service.go
- internal/catalogd/storage/localdir_test.go
- internal/catalogd/storage/localdir.go
- internal/catalogd/graphql/graphql.go
- internal/catalogd/server/handlers_test.go
| } | ||
|
|
||
| graphqlSvc := service.NewCachedGraphQLService() | ||
| rootURL, _ := url.Parse("/catalogs/") |
There was a problem hiding this comment.
Handle url.Parse errors instead of discarding them.
url.Parse returns an error and it is currently ignored. Please handle it explicitly and fail fast if parsing fails.
Suggested fix
- rootURL, _ := url.Parse("/catalogs/")
+ rootURL, err := url.Parse("/catalogs/")
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "invalid root URL: %v\n", err)
+ os.Exit(1)
+ }As per coding guidelines, **/*.go: “Never ignore error returns”.
🤖 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 `@hack/demo/graphql-demo-server/main.go` at line 51, The call to
url.Parse("/catalogs/") ignores the returned error; change the assignment to
capture the error (e.g., rootURL, err := url.Parse("/catalogs/")) and handle it
immediately (fail fast) by logging/terminating on error (e.g., log.Fatalf or
similar) so that rootURL is not used when parsing failed; update usage around
the rootURL variable accordingly.
Contributor
There was a problem hiding this comment.
nit for a barebones demo.
Discard.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List coderabbitai comments on PR `#738` to find the one to resolve
gh api repos/openshift/operator-framework-operator-controller/pulls/738/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:80]}'Length of output: 1797
🏁 Script executed:
#!/bin/bash
# Resolve the review comment id 3311523226
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDONAGfIM6BPn3K"}) {
thread {
isResolved
}
}
}'Length of output: 747
🏁 Script executed:
#!/bin/bash
# Get review thread IDs for PR `#738`
gh api graphql -f query='
{
repository(owner: "openshift", name: "operator-framework-operator-controller") {
pullRequest(number: 738) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
body
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].databaseId == 3311523226) | {threadId: .id, isResolved: .isResolved}'Length of output: 680
🏁 Script executed:
#!/bin/bash
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOJp67086FIQX5"}) {
thread {
isResolved
}
}
}'Length of output: 668
@grokspawn Acknowledged — fair point for a barebones demo. I wasn't able to resolve the thread programmatically due to permission constraints, but you can resolve it manually. I'll note this context for future reviews.
✏️ Learnings added
Learnt from: grokspawn
Repo: openshift/operator-framework-operator-controller PR: 738
File: hack/demo/graphql-demo-server/main.go:51-51
Timestamp: 2026-05-27T20:01:25.918Z
Learning: In `hack/demo/graphql-demo-server/main.go` (Go demo/hack script in operator-framework-operator-controller), the maintainer (grokspawn) considers strict Go error-handling guidelines (e.g., handling `url.Parse` errors) as unnecessary nits for barebones demo/hack code. Do not raise error-handling issues in files under `hack/demo/` for this repository.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
|
@openshift-bot: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Contributor
|
The graphql stuff is from upstream, and cannot be changed in this PR. @grokspawn would have to follow up. |
Contributor
|
/test |
Contributor
|
/test openshift-e2e-aws-techpreview |
Contributor
|
The graphql feature is enabled via a feature-gate, which should not be known downstream, so if the |
Contributor
|
/retitle OPRUN-4625: Synchronize From Upstream Repositories |
openshift-ci
Bot
changed the title
|
@openshift-bot: This pull request references OPRUN-4625 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
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
18 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The downstream repository has been updated with the following following upstream commits:
The
vendor/directory has been updated and the following commits were carried:@catalogd-updateThis pull request is expected to merge without any human intervention. If tests are failing here, changes must land upstream to fix any issues so that future downstreaming efforts succeed.
/assign @openshift/openshift-team-operator-runtime
Summary by CodeRabbit
New Features
Documentation
Examples / Demos
Tests
Chores