🌱 chore(deps): upgrade k8s to 1.35 and Go dependencies (edge)#2486
Open
camilamacedo86 wants to merge 9 commits intooperator-framework:mainfrom
Open
🌱 chore(deps): upgrade k8s to 1.35 and Go dependencies (edge)#2486camilamacedo86 wants to merge 9 commits intooperator-framework:mainfrom
camilamacedo86 wants to merge 9 commits intooperator-framework:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
deps-up
branch
from
c288cf6 to
41e7cc0
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Assisted-by: Cursor/Claude
…0.23.0 controller-runtime v0.23.0 introduced type-safe webhook builders using generics. Webhook methods now receive typed objects directly instead of runtime.Object, eliminating the need for type assertions and providing compile-time type safety. Changes: - Update ClusterCatalog webhook Default method to use *ocv1.ClusterCatalog - Update SetupWebhookWithManager to pass type to NewWebhookManagedBy - Simplify tests to leverage type safety (no type assertion tests needed) Assisted-by: Cursor/Claude
camilamacedo86
force-pushed
the
deps-up
branch
from
83cde26 to
a16409e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2486 +/- ##
==========================================
+ Coverage 69.73% 73.16% +3.42%
==========================================
Files 102 102
Lines 8471 8480 +9
==========================================
+ Hits 5907 6204 +297
+ Misses 2091 1798 -293
- Partials 473 478 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
deps-up
branch
from
f3bb698 to
f14866b
Compare
camilamacedo86
force-pushed
the
deps-up
branch
from
f14866b to
4a09f4b
Compare
camilamacedo86
changed the title
camilamacedo86
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
controller-runtime v0.23.0 changed test environment teardown timing behavior. Direct calls to testEnv.Stop() can fail intermittently due to graceful shutdown timing, requiring retry logic to ensure proper cleanup. Changes: - Add StopWithRetry helper function to test/utils.go with retry logic - Wrap testEnv.Stop() calls with retry helper (1-minute timeout, 1-second interval) - Add error logging for each retry attempt to aid debugging - Update api/v1/suite_test.go to use test.StopWithRetry - Update internal/operator-controller/controllers/suite_test.go to use test.StopWithRetry This shared utility follows DRY principle and ensures consistent teardown behavior across all test suites, preventing flaky test failures during teardown. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # api/v1/suite_test.go # internal/operator-controller/controllers/suite_test.go
…v0.23.0 controller-runtime v0.23.0 added Apply method to the StatusWriter interface for server-side apply support on status subresources. Changes: - Add Apply method to statusWriterMock to satisfy StatusWriter interface - Method signature: Apply(context.Context, runtime.ApplyConfiguration, ...SubResourceApplyOption) Assisted-by: Cursor/Claude
The client.Apply patch type constant is deprecated in controller-runtime v0.23.0+ in favor of typed Apply methods that require generated apply configurations. However, server-side apply with field ownership management is essential for this controller to properly manage ClusterExtensionRevision lifecycle. The deprecated client.Apply provides: - Field ownership via client.FieldOwner() - Force ownership via client.ForceOwnership - Automatic create-or-update semantics Alternative approaches (MergeFrom, StrategicMerge) lack field management support, causing upgrade test failures where ownership conflicts prevent proper updates during controller upgrades. Changes: - Keep client.Apply usage with comprehensive nolint explanation - Document why server-side apply semantics are required - Note migration path: generate apply configurations project-wide This ensures controllers work correctly during upgrades while documenting the technical debt for future migration. Assisted-by: Cursor/Claude
The fake.NewSimpleClientset is deprecated in client-go in favor of fake.NewClientset which provides better support for field management and server-side apply testing. The migration is straightforward for tests that don't require apply configurations - simply replace NewSimpleClientset() with NewClientset(). The new implementation works seamlessly with existing reactor-based mocking. Changes: - Replace fake.NewSimpleClientset() with fake.NewClientset() - Remove nolint suppression comment - All tests pass without modifications Assisted-by: Cursor/Claude
Address code review feedback by making the Apply method implementation more defensive: - Add applyCalled tracking field to statusWriterMock - Return error if Apply is unexpectedly called during tests - This ensures tests fail immediately if the code starts using Apply, rather than silently passing with incorrect assumptions The error message clearly indicates this method is not expected to be used, making debugging easier if the interface usage changes in the future. Assisted-by: Cursor/Claude
Controller-runtime v0.23.1+ has faster leader election startup, causing timing issues with the upgrade e2e tests that watch pod logs for leader election messages. The log watching approach fails because it uses Follow=true which only streams NEW logs, missing messages emitted before the test starts watching. With faster startup in v0.23.1+, leader election often completes before the test begins watching. Changes: - Replace log watching with direct lease object checking - Poll lease.Spec.HolderIdentity to confirm leader election occurred - More reliable than log watching, independent of timing - Add k8s.io/utils/ptr import for log options This approach is timing-independent and directly verifies the desired state rather than relying on log message observation. Assisted-by: Cursor/Claude Co-authored-by: Cursor <cursoragent@cursor.com>
camilamacedo86
force-pushed
the
deps-up
branch
from
4a09f4b to
e7a5d34
Compare
…ernetes v1.35
Kubernetes v1.35 changed CEL validation error message format to include
the actual validated value instead of just the type ("string" or "object").
This provides more helpful error messages for debugging validation failures.
This commit was accidentally dropped during interactive rebase.
Changes:
- Update test expectations to include actual values in error messages
- Change from 'Invalid value: "string"' to 'Invalid value: "actual-value"'
- Update 20+ test cases in TestImageSourceCELValidationRules and related tests
- Fix indentation consistency across all test cases
Assisted-by: Cursor/Claude
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
commented
Feb 6, 2026
| // Track in a future issue: "Generate apply configurations and migrate to typed Apply methods" | ||
| // | ||
| // nolint:staticcheck // SA1019: server-side apply required, needs generated apply configurations | ||
| return bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) |
Contributor
Author
There was a problem hiding this comment.
TODO: Lets looking on that in a follow up shows very complex to go in this PR
camilamacedo86
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Reviewer Checklist