NO-ISSUE: Synchronize From Upstream Repositories#754
Open
openshift-bot wants to merge 110 commits into
Open
Conversation
… (#2775) Introduce go.uber.org/mock (gomock) with mockgen code generation to replace all hand-written mock structs across the test suite. This eliminates duplicated mocks, establishes a single consistent mocking pattern, and ensures mocks stay in sync with interface changes automatically via make generate-mocks. Infrastructure: - Add mockgen via bingo with unversioned symlink for go generate - Add generate-mocks Makefile target (go generate ./...) - Wire into existing generate/verify targets for CI enforcement - Centralized //go:generate directives in internal/testutil/mock/ for exported interfaces (external and internal) - Source-mode directives at interface definitions for unexported interfaces (trackingCache, sourcerer), output to _test.go files to keep gomock out of the production binary - Mock helpers accept shared *gomock.Controller to avoid controller proliferation in nested mock scenarios Rename MockPuller/MockCache to FakePuller/FakeCache in image/fakes.go to clarify they are test fakes (preconfigured return values), not mocks (interaction verification). Remove duplicated FakeCertProvider from render/fake.go and rukpak/util/testing.go, replaced by generated MockCertificateProvider. FakeBundleSource (a function type, not an interface) is retained as-is. Co-authored-by: Claude <noreply@anthropic.com>
openshift-bot
added
tide/merge-method-merge
openshift-ci-robot
added
the
jira/valid-reference
openshift-bot
added
the
approved
|
@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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (32)
📒 Files selected for processing (61)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (25)
🚧 Files skipped from review as they are similar to previous changes (30)
WalkthroughAdds gomock tooling and generated mocks, then rewrites tests and helpers across catalog metadata, catalogd, operator-controller, and shared utilities to use the new mock packages and renamed fake types. ChangesGomock migration and mock generation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
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 |
1 similar comment
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 |
![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)
internal/catalogd/server/handlers_test.go (1)
25-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle
url.Parseerrors instead of discarding them.These setups ignore parse errors with
_; please assert them (or use a small helper) to keep test failures explicit and compliant.Suggested change pattern
-rootURL, _ := url.Parse("http://localhost/") +rootURL, err := url.Parse("http://localhost/") +require.NoError(t, err)As per path instructions,
**/*.go: Never ignore error returns.Also applies to: 44-45, 63-64, 82-83, 104-105, 125-126, 147-148, 206-207, 229-230, 256-257
🤖 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_test.go` around lines 25 - 26, The test setup is discarding the error return value from url.Parse by using the blank identifier. Replace each occurrence of url.Parse error being ignored with `_` by instead assigning the error to a variable and asserting it using a test assertion like require.NoError or similar. Apply this fix to all instances mentioned in the comment across the file (at lines around 25-26, 44-45, 63-64, 82-83, 104-105, 125-126, 147-148, 206-207, 229-230, and 256-257) to ensure parse errors are explicitly handled and test failures remain clear.Source: Path instructions
🤖 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 `@internal/catalogd/server/handlers_test.go`:
- Around line 132-135: The BodyTooLarge test is currently using an invalid JSON
body (just repeated "a" characters), which causes the handler to reject it for
malformed JSON rather than for exceeding the 1MB size limit. Replace the
largeBody with a valid JSON structure containing a small GraphQL query followed
by padding fields with large oversized content to exceed the 1MB limit. This
ensures the test actually exercises the max-body guard validation instead of
JSON parsing validation. Apply this fix to both the first test block (around the
largeBody assignment) and the second occurrence mentioned in the "also applies
to" section.
In `@internal/operator-controller/applier/boxcutter_test.go`:
- Around line 375-381: The mock expectation setup for the renderer's Get method
is using .AnyTimes() which allows zero invocations, meaning the assertions
inside the DoAndReturn callback will never execute if the code being tested
stops calling the renderer, causing the test to pass silently. Replace
.AnyTimes() with .Times(1) to enforce that the Get method must be called exactly
once, ensuring the parameter validation assertions are actually executed. Apply
this change to the Get method expectation around line 375 and also to the
similar mock expectations mentioned at lines 1199-1226.
In `@internal/operator-controller/applier/helm_test.go`:
- Line 722: The error return value from the helmApplier.Apply() call is being
discarded with blank identifiers, which means the test does not validate whether
the Apply operation succeeded or failed. Capture the error return value (the
third return value) from helmApplier.Apply into a named variable instead of
discarding it with underscore, then assert it using require.NoError(t, err) to
ensure the Apply operation completes without errors. This ensures the
integration test properly validates the success of the helm apply operation.
- Around line 335-358: In the test for the pre-authorization flow, replace the
`.AnyTimes()` call with `.Times(1)` in the mockPA.EXPECT().PreAuthorize mock
expectation to ensure the test explicitly verifies that the PreAuthorize method
is called exactly once. This strengthens the test for the security-critical
pre-authorization operation by catching any accidental changes to the code path
that might skip the authorization check, such as the removal of the nil check
before calling runPreAuthorizationChecks.
- Around line 297-299: The mockCache.Watch mock expectations are configured to
expect 3 arguments using Watch(gomock.Any(), gomock.Any(), gomock.Any()), but
the actual implementation calls cache.Watch(ctx, h.Watcher, relObjects...)
which, when relObjects is nil, results in only 2 arguments being passed. Fix
this by updating all mockCache.EXPECT().Watch() mock expectations throughout the
test file to accept only 2 arguments instead of 3 by changing from
Watch(gomock.Any(), gomock.Any(), gomock.Any()) to Watch(gomock.Any(),
gomock.Any()). Apply this fix to all occurrences mentioned: lines around
297-299, 509-511, 669-671, 701-703, and 731-733.
In `@internal/operator-controller/applier/provider_test.go`:
- Around line 239-241: The MockCertificateProvider created in the
RegistryV1ManifestProvider initialization is missing expected method calls.
Since the test enables webhook support with webhook definitions in the bundle,
the rendering process will invoke InjectCABundle, AdditionalObjects, and
GetCertSecretInfo on the certificate provider. Add EXPECT() calls on the
mockrender.NewMockCertificateProvider to set up expectations for these three
methods (InjectCABundle, AdditionalObjects, and GetCertSecretInfo), providing
appropriate return values for each to allow the webhook rendering to complete
successfully.
In `@internal/operator-controller/controllers/clusterobjectset_controller.go`:
- Line 61: The interface name `Sourcoser` appears to be a misspelling and should
be corrected. Rename the interface definition at line 333 from `Sourcoser` to
`Sourcer` (or another more conventional name like `SourceProvider` or
`SourceHandler` depending on its purpose), then update all references to this
interface throughout the codebase including the mockgen directive at line 61 in
the exclude_interfaces parameter to use the new corrected name consistently.
---
Nitpick comments:
In `@internal/catalogd/server/handlers_test.go`:
- Around line 25-26: The test setup is discarding the error return value from
url.Parse by using the blank identifier. Replace each occurrence of url.Parse
error being ignored with `_` by instead assigning the error to a variable and
asserting it using a test assertion like require.NoError or similar. Apply this
fix to all instances mentioned in the comment across the file (at lines around
25-26, 44-45, 63-64, 82-83, 104-105, 125-126, 147-148, 206-207, 229-230, and
256-257) to ensure parse errors are explicitly handled and test failures remain
clear.
🪄 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: 6bcda6be-e1a4-418a-8264-9725ebd325e7
⛔ Files ignored due to path filters (29)
.bingo/mockgen.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumopenshift/tests-extension/go.sumis excluded by!**/*.sumvendor/github.com/stretchr/objx/.codeclimate.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/Taskfile.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/accessors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/conversions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/mutations.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/security.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/tests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/type_specific.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/type_specific_codegen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/value.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/mock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/mock/mock.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/call.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/callset.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/controller.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/matchers.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/string.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (57)
.bingo/Variables.mk.bingo/mockgen.mod.bingo/variables.envAGENTS.mdMakefilego.modinternal/catalogd/controllers/core/clustercatalog_controller_test.gointernal/catalogd/server/handlers_test.gointernal/catalogd/serverutil/serverutil_test.gointernal/operator-controller/action/helm_test.gointernal/operator-controller/applier/boxcutter_test.gointernal/operator-controller/applier/helm_test.gointernal/operator-controller/applier/provider_test.gointernal/operator-controller/authorization/rbac_test.gointernal/operator-controller/catalogmetadata/client/client_test.gointernal/operator-controller/config/config_test.gointernal/operator-controller/contentmanager/cache/cache.gointernal/operator-controller/contentmanager/cache/cache_test.gointernal/operator-controller/contentmanager/cache/mock_sourcerer_gen_test.gointernal/operator-controller/controllers/clustercatalog_controller_test.gointernal/operator-controller/controllers/clusterextension_controller_test.gointernal/operator-controller/controllers/clusterobjectset_controller.gointernal/operator-controller/controllers/clusterobjectset_controller_internal_test.gointernal/operator-controller/controllers/clusterobjectset_controller_test.gointernal/operator-controller/controllers/mock_trackingcache_gen_test.gointernal/operator-controller/controllers/resolve_ref_test.gointernal/operator-controller/controllers/suite_test.gointernal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.gointernal/operator-controller/rukpak/render/certprovider_test.gointernal/operator-controller/rukpak/render/fake.gointernal/operator-controller/rukpak/render/registryv1/generators/generators_test.gointernal/operator-controller/rukpak/render/render_test.gointernal/operator-controller/rukpak/util/testing/testing.gointernal/shared/util/featuregates/logging_test.gointernal/shared/util/image/fakes.gointernal/shared/util/image/pull_test.gointernal/testutil/mock/applier/mock_applier.gointernal/testutil/mock/authorization/mock_authorization.gointernal/testutil/mock/catalogclient/mock_cache.gointernal/testutil/mock/catalogdserver/mock_catalogstore.gointernal/testutil/mock/catalogdservice/mock_graphqlservice.gointernal/testutil/mock/cmcache/mock_cache.gointernal/testutil/mock/config/mock_schemaprovider.gointernal/testutil/mock/contentmanager/mock_contentmanager.gointernal/testutil/mock/controllers/mock_controllers.gointernal/testutil/mock/crdclient/mock_crdinterface.gointernal/testutil/mock/ctrlclient/mock_client.gointernal/testutil/mock/generate.gointernal/testutil/mock/helmclient/interfaces.gointernal/testutil/mock/helmclient/mock_actionclient.gointernal/testutil/mock/helmclient/mock_composite.gointernal/testutil/mock/httputil/mock_roundtripper.gointernal/testutil/mock/logrsink/mock_logsink.gointernal/testutil/mock/machinery/mock_results.gointernal/testutil/mock/rbac/mock_rulesresolver.gointernal/testutil/mock/render/mock_certprovider.gointernal/testutil/mock/storage/mock_instance.go
💤 Files with no reviewable changes (2)
- internal/operator-controller/rukpak/render/fake.go
- internal/operator-controller/rukpak/util/testing/testing.go
Comment on lines
132
to
135
| // Create a body larger than 1MB | ||
| largeBody := strings.Repeat("a", 1<<20+1) | ||
| req := httptest.NewRequest(http.MethodPost, "/test-catalog/api/v1/graphql", strings.NewReader(largeBody)) | ||
| req.SetPathValue("catalog", "test-catalog") | ||
| w := httptest.NewRecorder() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
BodyTooLarge currently validates malformed JSON, not the max-body guard.
The request body is invalid from byte 1, so this test can pass without exercising the 1MB limit. Use valid JSON with a small query and oversized padding field.
Targeted fix
-// Create a body larger than 1MB
-largeBody := strings.Repeat("a", 1<<20+1)
-req := httptest.NewRequest(http.MethodPost, "/test-catalog/api/v1/graphql", strings.NewReader(largeBody))
+// Create a valid JSON body larger than 1MB
+largePadding := strings.Repeat("a", 1<<20+1)
+largeBody := `{"query":"{ summary { totalSchemas } }","padding":"` + largePadding + `"}`
+req := httptest.NewRequest(http.MethodPost, "/test-catalog/api/v1/graphql", strings.NewReader(largeBody))Also applies to: 137-142
🤖 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_test.go` around lines 132 - 135, The
BodyTooLarge test is currently using an invalid JSON body (just repeated "a"
characters), which causes the handler to reject it for malformed JSON rather
than for exceeding the 1MB size limit. Replace the largeBody with a valid JSON
structure containing a small GraphQL query followed by padding fields with large
oversized content to exceed the 1MB limit. This ensures the test actually
exercises the max-body guard validation instead of JSON parsing validation.
Apply this fix to both the first test block (around the largeBody assignment)
and the second occurrence mentioned in the "also applies to" section.
Comment on lines
+375
to
+381
| r.EXPECT().Get(dummyBundle, ext).DoAndReturn( | ||
| func(b fs.FS, e *ocv1.ClusterExtension) ([]client.Object, error) { | ||
| t.Log("by checking renderer was called with the correct parameters") | ||
| require.Equal(t, dummyBundle, b) | ||
| require.Equal(t, ext, e) | ||
| return nil, nil | ||
| }, | ||
| } | ||
| }).AnyTimes() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 246
🏁 Script executed:
# Examine the code at lines 375-381
sed -n '370,385p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 691
🏁 Script executed:
# Examine the code at lines 1199-1226
sed -n '1195,1230p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1794
🏁 Script executed:
# Examine the code at lines 1298-1300
sed -n '1290,1310p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1062
🏁 Script executed:
# Let me check the context around these mocks to understand what they're testing
# Get more context around line 375
sed -n '350,390p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1627
🏁 Script executed:
# Check context for the second location (preauthorizer test around 1199)
sed -n '1170,1235p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 2822
🏁 Script executed:
# Check if there are any other AnyTimes() + DoAndReturn patterns in this file
rg -n 'DoAndReturn.*\.AnyTimes\(\)' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Also search for Times(1) patterns to see if there's consistency
rg -n 'DoAndReturn.*\.Times' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Let me search for the actual test execution to understand better
# Look at what happens after these mocks are set up
sed -n '375,410p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1177
🏁 Script executed:
# For the second case, see the actual test invocation
sed -n '1225,1310p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 3555
🏁 Script executed:
# Search for other DoAndReturn patterns in the file to understand the style
rg -n 'EXPECT.*DoAndReturn' internal/operator-controller/applier/boxcutter_test.go | head -20Repository: openshift/operator-framework-operator-controller
Length of output: 1295
🏁 Script executed:
# Let me check the test invocations to confirm mocks are actually called
# Look at what Test_SimpleRevisionGenerator_Renderer_Integration does after setup
sed -n '358,395p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1469
🏁 Script executed:
# Check what the table-driven test does with the preAuthorizer mock around 1270
sed -n '1265,1290p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1230
🏁 Script executed:
# Look for other patterns where AnyTimes is paired with assertions
rg -B5 -A10 'DoAndReturn.*require\.' internal/operator-controller/applier/boxcutter_test.go | head -50Repository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Let me see the full test invocation around line 1270+
sed -n '1268,1295p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1320
🏁 Script executed:
# Check if PreAuthorize is actually called in the Boxcutter Apply/methods
rg -n 'pa\.PreAuthorize|preAuthorizer\.PreAuthorize' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Look at what happens in the actual boxcutter.Apply or similar methods
rg -n 'PreAuthorize' internal/operator-controller/applier/boxcutter.go | head -10Repository: openshift/operator-framework-operator-controller
Length of output: 517
🏁 Script executed:
# Let me see the actual test execution that follows the mock setup
sed -n '1268,1320p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 2254
🏁 Script executed:
# Check if there are any conditional paths that could skip calling PreAuthorize
sed -n '640,660p' internal/operator-controller/applier/boxcutter.goRepository: openshift/operator-framework-operator-controller
Length of output: 1215
🏁 Script executed:
# Check what's at lines 1298-1300 to confirm the third location
sed -n '1295,1310p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 762
🏁 Script executed:
# Verify the GenerateRevision method signature and confirm it calls the mock
rg -A5 'func.*GenerateRevision' internal/operator-controller/applier/simple_revision_generator.go | head -20Repository: openshift/operator-framework-operator-controller
Length of output: 292
🏁 Script executed:
# Find the file with GenerateRevision
fd -type f -name "*.go" internal/operator-controller/applier/ | xargs grep -l "GenerateRevision"Repository: openshift/operator-framework-operator-controller
Length of output: 361
🏁 Script executed:
# Check the test around line 1298-1300
sed -n '1296,1305p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 636
🏁 Script executed:
# Find and check the GenerateRevision calls to understand the code structure
find internal/operator-controller/applier -name "*.go" | head -5Repository: openshift/operator-framework-operator-controller
Length of output: 443
🏁 Script executed:
# Look for the actual location line 1298-1300
sed -n '1295,1305p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 643
🏁 Script executed:
# Check a broader context to find all the locations mentioned in "Also applies to"
rg -n '\.AnyTimes\(\)' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 2665
🏁 Script executed:
# Get exact context around line 1298-1300
sed -n '1298,1305p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 501
🏁 Script executed:
# Also check lines 1199-1226 more carefully to see if there are assertions
sed -n '1199,1230p' internal/operator-controller/applier/boxcutter_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1575
Use .Times(1) instead of .AnyTimes() for mocks that verify call arguments.
Assertions inside DoAndReturn only execute if the mock is invoked. .AnyTimes() permits zero calls, so if the renderer or pre-authorizer stops being called, these assertions never run and tests pass silently. Use .Times(1) to enforce that the call must occur.
Suggested fix
- }).AnyTimes()
+ }).Times(1)Also applies to lines 1199–1226.
🤖 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/operator-controller/applier/boxcutter_test.go` around lines 375 -
381, The mock expectation setup for the renderer's Get method is using
.AnyTimes() which allows zero invocations, meaning the assertions inside the
DoAndReturn callback will never execute if the code being tested stops calling
the renderer, causing the test to pass silently. Replace .AnyTimes() with
.Times(1) to enforce that the Get method must be called exactly once, ensuring
the parameter validation assertions are actually executed. Apply this change to
the Get method expectation around line 375 and also to the similar mock
expectations mentioned at lines 1199-1226.
Comment on lines
+335
to
+358
| mockPA.EXPECT().PreAuthorize(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( | ||
| func(ctx context.Context, userInfo user.Info, reader io.Reader, additionalRequiredPerms ...authorization.UserAuthorizerAttributesFactory) ([]authorization.ScopedPolicyRules, error) { | ||
| t.Log("has correct user") | ||
| require.Equal(t, "system:serviceaccount:test-namespace:test-sa", userInfo.GetName()) | ||
| require.Empty(t, userInfo.GetUID()) | ||
| require.Nil(t, userInfo.GetExtra()) | ||
| require.Empty(t, userInfo.GetGroups()) | ||
|
|
||
| t.Log("has correct additional permissions") | ||
| require.Len(t, additionalRequiredPerms, 1) | ||
| perms := additionalRequiredPerms[0](userInfo) | ||
|
|
||
| require.Len(t, perms, 1) | ||
| require.Equal(t, authorizer.AttributesRecord{ | ||
| User: userInfo, | ||
| Name: "test-ext", | ||
| APIGroup: "olm.operatorframework.io", | ||
| APIVersion: "v1", | ||
| Resource: "clusterextensions/finalizers", | ||
| ResourceRequest: true, | ||
| Verb: "update", | ||
| }, perms[0]) | ||
| return nil, nil | ||
| }).AnyTimes() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and verify line counts
wc -l internal/operator-controller/applier/helm_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 240
🏁 Script executed:
# Read the specific lines mentioned (335-358 and 503-504)
sed -n '330,365p' internal/operator-controller/applier/helm_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1871
🏁 Script executed:
# Check lines 503-504 context
sed -n '495,510p' internal/operator-controller/applier/helm_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 905
🏁 Script executed:
# Get broader context around line 335 to understand the test function
sed -n '310,370p' internal/operator-controller/applier/helm_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 2754
🏁 Script executed:
# Get broader context around line 503 to understand that test function
sed -n '480,520p' internal/operator-controller/applier/helm_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1868
🏁 Script executed:
# Search for all PreAuthorize mock expectations to see the pattern usage
rg "PreAuthorize.*AnyTimes" internal/operator-controller/applier/helm_test.go -B 5 -A 1Repository: openshift/operator-framework-operator-controller
Length of output: 1456
🏁 Script executed:
# Check the actual implementation to see if PreAuthorize should always be called
rg -A 10 "func.*PreAuthorize" internal/operator-controller/applier/ --type go | head -40Repository: openshift/operator-framework-operator-controller
Length of output: 4040
🏁 Script executed:
# Look at the helm applier code to see where PreAuthorize is called
sed -n '1,100p' internal/operator-controller/applier/helm.go | grep -A 5 -B 5 "PreAuthorize"Repository: openshift/operator-framework-operator-controller
Length of output: 1664
🏁 Script executed:
# Check what the test actually calls and whether PreAuthorize should be called
sed -n '358,380p' internal/operator-controller/applier/helm_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1103
🏁 Script executed:
# Look for similar test patterns in boxcutter_test.go to see if they use .Times(1)
rg "PreAuthorize.*Times\(1\)" internal/operator-controller/applier/ -B 2 -A 2Repository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Check if there are any tests that properly enforce PreAuthorize call count
grep -n "\.Times(" internal/operator-controller/applier/helm_test.go | head -20Repository: openshift/operator-framework-operator-controller
Length of output: 271
🏁 Script executed:
# Check the Apply function to see if PreAuthorize is always called or conditional
sed -n '40,150p' internal/operator-controller/applier/helm.go | grep -A 50 "func (h \*Helm) Apply"Repository: openshift/operator-framework-operator-controller
Length of output: 1709
🏁 Script executed:
# Look for where runPreAuthorizationChecks is called
rg "runPreAuthorizationChecks" internal/operator-controller/applier/helm.go -B 3 -A 3Repository: openshift/operator-framework-operator-controller
Length of output: 1002
🏁 Script executed:
# Check if PreAuthorizer can be nil
rg "PreAuthorizer.*nil" internal/operator-controller/applier/ --type goRepository: openshift/operator-framework-operator-controller
Length of output: 340
Use .Times(1) instead of .AnyTimes() for parameter-checking and success-path pre-authorization tests to ensure the security gate is properly enforced.
The Apply method in helm.go checks if h.PreAuthorizer != nil before calling runPreAuthorizationChecks, so these tests do invoke PreAuthorize with the current code. However, using .AnyTimes() allows the mock to be satisfied even if the call is skipped entirely, which weakens the test. For security-critical operations like pre-authorization, explicitly asserting .Times(1) will catch if the nil check is accidentally removed, the PreAuthorizer is not assigned, or the code path changes unexpectedly.
🤖 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/operator-controller/applier/helm_test.go` around lines 335 - 358, In
the test for the pre-authorization flow, replace the `.AnyTimes()` call with
`.Times(1)` in the mockPA.EXPECT().PreAuthorize mock expectation to ensure the
test explicitly verifies that the PreAuthorize method is called exactly once.
This strengthens the test for the security-critical pre-authorization operation
by catching any accidental changes to the code path that might skip the
authorization check, such as the removal of the nil check before calling
runPreAuthorizationChecks.
Comment on lines
+239
to
+241
| ctrl := gomock.NewController(t) | ||
| provider := applier.RegistryV1ManifestProvider{ | ||
| CertificateProvider: FakeCertProvider{}, | ||
| CertificateProvider: mockrender.NewMockCertificateProvider(ctrl), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file around lines 239-241
cd /tmp && find . -name "provider_test.go" -path "*/applier/*" 2>/dev/null | head -5Repository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Get the repository structure to understand the codebase
git ls-files | grep -E "provider_test.go|provider\.go" | grep applierRepository: openshift/operator-framework-operator-controller
Length of output: 289
🏁 Script executed:
# Look at the test file context around lines 239-241
sed -n '230,260p' internal/operator-controller/applier/provider_test.goRepository: openshift/operator-framework-operator-controller
Length of output: 1102
🏁 Script executed:
# Find the CertificateProvider interface definition
rg "type CertificateProvider interface" --type go -A 10Repository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Check what methods are called on the certificate provider in the applier code
rg "CertificateProvider" --type go -B 2 -A 2 | head -100Repository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Look for webhook-related code in the applier package
rg "webhook" --type go -i -B 3 -A 3 internal/operator-controller/applier/Repository: openshift/operator-framework-operator-controller
Length of output: 9240
🏁 Script executed:
# Search for where CertificateProvider methods are actually called in the applier/render code
rg "CertificateProvider\." --type go -B 3 -A 3 internal/operator-controller/Repository: openshift/operator-framework-operator-controller
Length of output: 186
🏁 Script executed:
# Look at the render package to find CertificateProvider interface and its usage
fd "render" -type d internal/Repository: openshift/operator-framework-operator-controller
Length of output: 357
🏁 Script executed:
# Check what the BundleRenderer does and whether it uses the CertificateProvider
rg "BundleRenderer" --type go -B 2 -A 5 internal/operator-controller/applier/ | head -80Repository: openshift/operator-framework-operator-controller
Length of output: 7508
🏁 Script executed:
# Find the CertificateProvider interface definition in the render package
find internal -name "*.go" -type f | xargs grep -l "type CertificateProvider"Repository: openshift/operator-framework-operator-controller
Length of output: 245
🏁 Script executed:
# Check the actual render package to understand the interface
rg "type CertificateProvider interface" -A 10 internal/Repository: openshift/operator-framework-operator-controller
Length of output: 1317
🏁 Script executed:
# Find where InjectCABundle, AdditionalObjects, and GetCertSecretInfo are called
rg "InjectCABundle|AdditionalObjects|GetCertSecretInfo" --type go -B 3 -A 3 internal/operator-controller/Repository: openshift/operator-framework-operator-controller
Length of output: 35226
🏁 Script executed:
# Check what the BundleRenderer.Render method does, especially for webhooks
cat -n internal/operator-controller/rukpak/render/render.go | head -150Repository: openshift/operator-framework-operator-controller
Length of output: 6009
🏁 Script executed:
# Look for webhook-specific rendering logic
rg "webhook" --type go -i -B 5 -A 5 internal/operator-controller/rukpak/render/ | head -150Repository: openshift/operator-framework-operator-controller
Length of output: 16858
Set expectations on the MockCertificateProvider when webhook rendering is tested.
The test enables webhook support and includes webhook definitions in the bundle, triggering webhook resource generation. The generators call InjectCABundle, AdditionalObjects, and GetCertSecretInfo on the certificate provider during rendering. A gomock created without expectations will fail with unexpected call errors. Add the required method expectations using EXPECT() or provide a benign implementation for this success path.
🤖 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/operator-controller/applier/provider_test.go` around lines 239 -
241, The MockCertificateProvider created in the RegistryV1ManifestProvider
initialization is missing expected method calls. Since the test enables webhook
support with webhook definitions in the bundle, the rendering process will
invoke InjectCABundle, AdditionalObjects, and GetCertSecretInfo on the
certificate provider. Add EXPECT() calls on the
mockrender.NewMockCertificateProvider to set up expectations for these three
methods (InjectCABundle, AdditionalObjects, and GetCertSecretInfo), providing
appropriate return values for each to allow the webhook rendering to complete
successfully.
| Clock clock.Clock | ||
| } | ||
|
|
||
| //go:generate mockgen -source clusterobjectset_controller.go -destination mock_trackingcache_gen_test.go -package controllers -mock_names trackingCache=MockTrackingCache -exclude_interfaces Sourcoser |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search clusterobjectset_controller.go for interface definitions that might match the excluded name.
echo "=== Looking for 'Sourcoser' or similar interface definitions ==="
grep -nE '^\s*(type|interface)\s+(Sourc|sourc)' internal/operator-controller/controllers/clusterobjectset_controller.go || echo "No matches found"
echo ""
echo "=== Listing all exported interfaces in the file ==="
grep -nE '^\s*type\s+[A-Z]\w+\s+interface' internal/operator-controller/controllers/clusterobjectset_controller.go || echo "No exported interfaces found"Repository: openshift/operator-framework-operator-controller
Length of output: 366
🏁 Script executed:
# Read the interface definition and surrounding context
sed -n '325,345p' internal/operator-controller/controllers/clusterobjectset_controller.go
# Also search for any other similar interface names
echo ""
echo "=== Searching for similar interface names ==="
grep -nE '\bSourc' internal/operator-controller/controllers/clusterobjectset_controller.go | head -20Repository: openshift/operator-framework-operator-controller
Length of output: 1364
🏁 Script executed:
# Check if there's a 'Sourcer' interface anywhere that might be the intended name
echo "=== Searching for 'Sourcer' (without 'o') ==="
grep -rn "Sourcer" internal/operator-controller/ --include="*.go" | head -20
# Also check the import statements and types to understand the interface better
echo ""
echo "=== Check interface usage context ==="
sed -n '354,365p' internal/operator-controller/controllers/clusterobjectset_controller.goRepository: openshift/operator-framework-operator-controller
Length of output: 3445
🏁 Script executed:
# Check what trackingCache actually is - what type implements Sourcoser
echo "=== Looking for trackingCache field definition in ClusterObjectSetReconciler ==="
grep -n "trackingCache\|TrackingCache" internal/operator-controller/controllers/clusterobjectset_controller.go | head -20
# Check the struct definition
echo ""
echo "=== Looking for ClusterObjectSetReconciler struct ==="
sed -n '1,100p' internal/operator-controller/controllers/clusterobjectset_controller.go | grep -A 20 "type ClusterObjectSetReconciler"Repository: openshift/operator-framework-operator-controller
Length of output: 2072
🏁 Script executed:
# Check for any comments or documentation about the Sourcoser interface
echo "=== Looking for comments or documentation about Sourcoser ==="
sed -n '328,338p' internal/operator-controller/controllers/clusterobjectset_controller.go
# Also check if Sourcoser is used elsewhere
echo ""
echo "=== Check all usages of Sourcoser ==="
grep -rn "Sourcoser" internal/operator-controller/ --include="*.go"Repository: openshift/operator-framework-operator-controller
Length of output: 1268
Sourcoser is defined but has an unusual name that warrants clarification.
The interface Sourcoser does exist at line 333 and is correctly referenced in the mockgen directive. However, the name itself is suspicious—it doesn't follow standard Go naming conventions and appears to be a misspelling. No interface named Sourcer exists in the codebase. Consider renaming to something more conventional (e.g., Sourcer, SourceProvider, or SourceHandler) or, if the current name is intentional, add a comment documenting its purpose.
🤖 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/operator-controller/controllers/clusterobjectset_controller.go` at
line 61, The interface name `Sourcoser` appears to be a misspelling and should
be corrected. Rename the interface definition at line 333 from `Sourcoser` to
`Sourcer` (or another more conventional name like `SourceProvider` or
`SourceHandler` depending on its purpose), then update all references to this
interface throughout the codebase including the mockgen directive at line 61 in
the exclude_interfaces parameter to use the new corrected name consistently.
Bumps [github.com/containerd/containerd](https://github.com/containerd/containerd) from 1.7.32 to 1.7.33. - [Release notes](https://github.com/containerd/containerd/releases) - [Changelog](https://github.com/containerd/containerd/blob/main/RELEASES.md) - [Commits](containerd/containerd@v1.7.32...v1.7.33) --- updated-dependencies: - dependency-name: github.com/containerd/containerd dependency-version: 1.7.33 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 [codecov/codecov-action](https://github.com/codecov/codecov-action) from 6.0.1 to 7.0.0. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v6.0.1...v7.0.0) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: 7.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [beautifulsoup4](https://www.crummy.com/software/BeautifulSoup/bs4/) from 4.14.3 to 4.15.0. --- updated-dependencies: - dependency-name: beautifulsoup4 dependency-version: 4.15.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>
openshift-bot
force-pushed
the
synchronize-upstream
branch
from
90ad5d5 to
87d6551
Compare
Contributor
|
New changes are detected. LGTM label has been removed. |
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 6 to 6.2.0. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v6...v6.2.0) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: 6.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>
openshift-bot
force-pushed
the
synchronize-upstream
branch
from
87d6551 to
d97c384
Compare
Contributor
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
304-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the package exclusion expression.
The same
go list | grep -vE ...filter appears twice; extracting it into one variable avoids drift betweenUNIT_TEST_DIRSandCOVERAGE_PKGS.Proposed refactor
-UNIT_TEST_DIRS := $(shell go list ./... | grep -vE "/test/|/testutils|/testutil/mock") $(shell go list ./test/internal/...) -COVERAGE_PKGS := $(shell go list ./... | grep -vE "/test/|/testutils|/testutil/mock" | paste -sd, -) +UNIT_TEST_EXCLUDE_RE := /test/|/testutils|/testutil/mock +UNIT_TEST_BASE_PKGS := $(shell go list ./... | grep -vE "$(UNIT_TEST_EXCLUDE_RE)") +UNIT_TEST_DIRS := $(UNIT_TEST_BASE_PKGS) $(shell go list ./test/internal/...) +COVERAGE_PKGS := $(shell printf '%s\n' $(UNIT_TEST_BASE_PKGS) | paste -sd, -)🤖 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 `@Makefile` around lines 304 - 305, Deduplicate the repeated package exclusion logic used by UNIT_TEST_DIRS and COVERAGE_PKGS by extracting the shared go list | grep -vE filter into a single Makefile variable, then reuse that variable in both definitions. Keep the existing package selection behavior unchanged, and make sure the new shared variable is the single source of truth for the exclusion pattern.
🤖 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.
Nitpick comments:
In `@Makefile`:
- Around line 304-305: Deduplicate the repeated package exclusion logic used by
UNIT_TEST_DIRS and COVERAGE_PKGS by extracting the shared go list | grep -vE
filter into a single Makefile variable, then reuse that variable in both
definitions. Keep the existing package selection behavior unchanged, and make
sure the new shared variable is the single source of truth for the exclusion
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ff043674-24c4-4fd5-93e7-d610a95776d2
⛔ Files ignored due to path filters (32)
.bingo/mockgen.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumopenshift/tests-extension/go.sumis excluded by!**/*.sumvendor/github.com/containerd/containerd/labels/labels.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/containerd/containerd/labels/validate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/containerd/containerd/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/.codeclimate.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/Taskfile.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/accessors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/conversions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/mutations.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/security.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/tests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/type_specific.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/type_specific_codegen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/objx/value.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/mock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/stretchr/testify/mock/mock.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/call.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/callset.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/controller.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/matchers.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/string.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (58)
.bingo/Variables.mk.bingo/mockgen.mod.bingo/variables.envAGENTS.mdMakefilego.modinternal/catalogd/controllers/core/clustercatalog_controller_test.gointernal/catalogd/server/handlers_test.gointernal/catalogd/serverutil/serverutil_test.gointernal/operator-controller/action/helm_test.gointernal/operator-controller/applier/boxcutter_test.gointernal/operator-controller/applier/helm_test.gointernal/operator-controller/applier/provider_test.gointernal/operator-controller/authorization/rbac_test.gointernal/operator-controller/catalogmetadata/client/client_test.gointernal/operator-controller/config/config_test.gointernal/operator-controller/contentmanager/cache/cache.gointernal/operator-controller/contentmanager/cache/cache_test.gointernal/operator-controller/contentmanager/cache/mock_sourcerer_gen_test.gointernal/operator-controller/controllers/clustercatalog_controller_test.gointernal/operator-controller/controllers/clusterextension_controller_test.gointernal/operator-controller/controllers/clusterobjectset_controller.gointernal/operator-controller/controllers/clusterobjectset_controller_internal_test.gointernal/operator-controller/controllers/clusterobjectset_controller_test.gointernal/operator-controller/controllers/mock_trackingcache_gen_test.gointernal/operator-controller/controllers/resolve_ref_test.gointernal/operator-controller/controllers/suite_test.gointernal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.gointernal/operator-controller/rukpak/render/certprovider_test.gointernal/operator-controller/rukpak/render/fake.gointernal/operator-controller/rukpak/render/registryv1/generators/generators_test.gointernal/operator-controller/rukpak/render/render_test.gointernal/operator-controller/rukpak/util/testing/testing.gointernal/shared/util/featuregates/logging_test.gointernal/shared/util/image/fakes.gointernal/shared/util/image/pull_test.gointernal/testutil/mock/applier/mock_applier.gointernal/testutil/mock/authorization/mock_authorization.gointernal/testutil/mock/catalogclient/mock_cache.gointernal/testutil/mock/catalogdserver/mock_catalogstore.gointernal/testutil/mock/catalogdservice/mock_graphqlservice.gointernal/testutil/mock/cmcache/mock_cache.gointernal/testutil/mock/config/mock_schemaprovider.gointernal/testutil/mock/contentmanager/mock_contentmanager.gointernal/testutil/mock/controllers/mock_controllers.gointernal/testutil/mock/crdclient/mock_crdinterface.gointernal/testutil/mock/ctrlclient/mock_client.gointernal/testutil/mock/generate.gointernal/testutil/mock/helmclient/interfaces.gointernal/testutil/mock/helmclient/mock_actionclient.gointernal/testutil/mock/helmclient/mock_composite.gointernal/testutil/mock/httputil/mock_roundtripper.gointernal/testutil/mock/logrsink/mock_logsink.gointernal/testutil/mock/machinery/mock_results.gointernal/testutil/mock/rbac/mock_rulesresolver.gointernal/testutil/mock/render/mock_certprovider.gointernal/testutil/mock/storage/mock_instance.gorequirements.txt
💤 Files with no reviewable changes (2)
- internal/operator-controller/rukpak/render/fake.go
- internal/operator-controller/rukpak/util/testing/testing.go
✅ Files skipped from review due to trivial changes (28)
- internal/testutil/mock/helmclient/interfaces.go
- .bingo/variables.env
- internal/testutil/mock/catalogclient/mock_cache.go
- .bingo/mockgen.mod
- requirements.txt
- internal/testutil/mock/config/mock_schemaprovider.go
- internal/testutil/mock/httputil/mock_roundtripper.go
- internal/testutil/mock/authorization/mock_authorization.go
- internal/testutil/mock/catalogdservice/mock_graphqlservice.go
- internal/testutil/mock/contentmanager/mock_contentmanager.go
- internal/testutil/mock/crdclient/mock_crdinterface.go
- internal/testutil/mock/storage/mock_instance.go
- internal/operator-controller/controllers/mock_trackingcache_gen_test.go
- internal/operator-controller/contentmanager/cache/cache.go
- internal/testutil/mock/render/mock_certprovider.go
- internal/shared/util/image/pull_test.go
- internal/testutil/mock/cmcache/mock_cache.go
- internal/operator-controller/controllers/clusterobjectset_controller.go
- internal/testutil/mock/logrsink/mock_logsink.go
- internal/testutil/mock/catalogdserver/mock_catalogstore.go
- internal/testutil/mock/controllers/mock_controllers.go
- internal/operator-controller/contentmanager/cache/mock_sourcerer_gen_test.go
- internal/testutil/mock/helmclient/mock_actionclient.go
- internal/testutil/mock/machinery/mock_results.go
- AGENTS.md
- internal/testutil/mock/ctrlclient/mock_client.go
- internal/testutil/mock/rbac/mock_rulesresolver.go
- internal/testutil/mock/applier/mock_applier.go
🚧 Files skipped from review as they are similar to previous changes (27)
- .bingo/Variables.mk
- internal/testutil/mock/helmclient/mock_composite.go
- internal/operator-controller/rukpak/render/certprovider_test.go
- internal/shared/util/image/fakes.go
- internal/operator-controller/rukpak/render/render_test.go
- go.mod
- internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go
- internal/testutil/mock/generate.go
- internal/operator-controller/controllers/suite_test.go
- internal/operator-controller/action/helm_test.go
- internal/operator-controller/controllers/clusterobjectset_controller_internal_test.go
- internal/operator-controller/contentmanager/cache/cache_test.go
- internal/operator-controller/controllers/resolve_ref_test.go
- internal/catalogd/serverutil/serverutil_test.go
- internal/operator-controller/controllers/clustercatalog_controller_test.go
- internal/operator-controller/applier/provider_test.go
- internal/operator-controller/authorization/rbac_test.go
- internal/operator-controller/config/config_test.go
- internal/catalogd/server/handlers_test.go
- internal/shared/util/featuregates/logging_test.go
- internal/operator-controller/rukpak/render/registryv1/generators/generators_test.go
- internal/catalogd/controllers/core/clustercatalog_controller_test.go
- internal/operator-controller/applier/boxcutter_test.go
- internal/operator-controller/catalogmetadata/client/client_test.go
- internal/operator-controller/controllers/clusterobjectset_controller_test.go
- internal/operator-controller/applier/helm_test.go
- internal/operator-controller/controllers/clusterextension_controller_test.go
Contributor
|
@openshift-bot: The following tests failed, say
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. |
…#2782) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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>
…t in OTE tests Update all remaining references to ClusterExtensionRevision in openshift/tests-extension to use ClusterObjectSet, matching the upstream rename in operator-framework/operator-controller#2589. Files updated: - test/qe/specs/olmv1_ce.go: RBAC resource names and comments - test/olmv1-preflight.go: scenario constants, test names, RBAC rules - .openshift-tests-extension/openshift_payload_olmv1.json: test name - pkg/bindata/qe/bindata.go: embedded RBAC templates - test/qe/testdata/olm/sa-nginx-limited-boxcutter.yaml: RBAC resources - test/qe/testdata/olm/sa-nginx-insufficient-operand-rbac-boxcutter.yaml: RBAC resources Signed-off-by: Camila Macedo <cmacedo@redhat.com> Made-with: Cursor
…s ClusterObjectSet The upstream rename of ClusterExtensionRevision to ClusterObjectSet (operator-framework/operator-controller#2589) breaks the incompatible operator detection in cluster-olm-operator. The cluster-olm-operator binary still reads ClusterExtensionRevision resources to find operators with olm.maxOpenShiftVersion, so after the rename it never detects incompatible operators and InstalledOLMOperatorsUpgradeable stays True. Skip this test when NewOLMBoxCutterRuntime feature gate is enabled until cluster-olm-operator is updated to read ClusterObjectSet. Signed-off-by: Camila Macedo <cmacedo@redhat.com> Made-with: Cursor
Signed-off-by: Francesco Giudici <fgiudici@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>
Adds a new test that verifies cluster-olm-operator correctly configures operator-controller and catalogd deployments based on the cluster's control plane topology: - HA topologies (HighlyAvailable, HighlyAvailableArbiter, DualReplica): replicas=2 with a PodDisruptionBudget present - Non-HA topologies (SingleReplica/SNO, External): replicas=1, no PDB Also registers policyv1 in the test scheme to support PDB list queries. Assisted-by: claude Signed-off-by: Todd Short <tshort@redhat.com>
… builders Signed-off-by: Todd Short <tshort@redhat.com>
…ge to be consistent with ART for 5.0 Reconciling with https://github.com/openshift-eng/ocp-build-data/tree/7691ed4dc0b6585b358f9e73fb736ace9a48a286/images/ose-olm-operator-controller.yml
Set catalog image tags to v5.0 for the 4.23/5.0 release. Dynamically discover an installable package from the serving catalogs instead of hardcoding quay-operator v3.13.10, preferring quay-operator, cluster-logging, serverless-operator, logic-operator in that order then alling back to the first available package. Signed-off-by: Todd Short <tshort@redhat.com>
openshift-bot
force-pushed
the
synchronize-upstream
branch
from
d97c384 to
fbaa792
Compare
Contributor
|
New changes are detected. LGTM label has been removed. |
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
Documentation
Tests
Chores