Skip to content

Bump controller-runtime to v0.14.1 and k8s API to v0.26.1 #1060

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 3, 2023

Conversation

amisevsk
Copy link
Collaborator

@amisevsk amisevsk commented Mar 1, 2023

What does this PR do?

Updates core dependencies:
* sigs.k8s.io/controller-runtime to v0.14.1
* k8s.io/api (et al.) to v0.26.1

Notable changes in the update:

  • corev1.Handler is replaced with corev1.LifecycleHandler and corev1.ProbeHandler
  • k8s.io/apimachinery/pkg/util/clock is replaced with k8s.io/utils/clock
  • github.com/go-logr/logr/testing is deprecated in favor of github.com/go-logr/logr/testr

The update also ran into issues around github.com/redhat-cop/operator-utils, the latest version of which depends on k8s API v0.25.0 and controller-runtime v0.13.0. Since our usages of this package have been integrated into controller-runtime, the dependency is dropped in favor of these methods (latest versions of operator-utils just wrap the controller-runtime equivalents).

What issues does this PR fix or reference?

Closes #1059

Is it tested? How?

N/A

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

@amisevsk amisevsk requested a review from AObuchow March 1, 2023 21:10
@amisevsk amisevsk requested a review from ibuziuk as a code owner March 1, 2023 21:10
@openshift-ci openshift-ci bot added the approved label Mar 1, 2023
@amisevsk
Copy link
Collaborator Author

amisevsk commented Mar 1, 2023

Before merging this PR, we should do a smoke test against the Che Operator next to ensure the updated k8s API version does not cause issues.

@AObuchow
Copy link
Collaborator

AObuchow commented Mar 1, 2023

Code looks good to me 😎. I had experimented recently with upgrading the controller-runtime to v0.14.1 and am familiar with the API changes that were required (corev1.LifecycleHandler, corev1.ProbeHandler, k8s.io/utils/clock and github.com/go-logr/logr/testr).

Before merging this PR, we should do a smoke test against the Che Operator next to ensure the updated k8s API version does not cause issues.

I'm currently spinning up a cluster to test this out before approving this PR.

Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mock client pkg/library/flatten/internal/testutil/ needs to be modified so that the get function has the opts ...client.GetOption parameter.

And testr.New(t) needs to be used here and here

@AObuchow
Copy link
Collaborator

AObuchow commented Mar 2, 2023

I also compiled DWO (..even with the issues in the test files somehow? Maybe go doesn't care if there's compilation issues in the test files) and created + subscribed to the catalog source, and then deployed che:next with chectl and opened up the Che-Website workspace and things worked fine. Though I will repeat this process once the small final changes are made to this PR :)

amisevsk added 5 commits March 2, 2023 17:05
Update:
  * sigs.k8s.io/controller-runtime to v0.14.1
  * k8s.io/api (et al.) to v0.26.1

This update matches versions for the Kubernetes API and controller
runtime with devfile/api.

Signed-off-by: Angel Misevski <[email protected]>
* Replace corev1.Handler with corev1.LifecycleHandler or ProbeHandler as
  required
* Replace k8s.io/apimachinery/pkg/util/clock with k8s.io/utils/clock

Signed-off-by: Angel Misevski <[email protected]>
The functionality from operator-utils is now available in
controller-runtime, making the additional dependency unnecessary as its
functions just wrap controller-runtime

The operator-utils dependency makes updating controller-runtime
difficult as operator-utils itself depends on k8s.io/api and
controller-runtime.

Signed-off-by: Angel Misevski <[email protected]>
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Patch coverage: 72.72% and project coverage change: -0.05 ⚠️

Comparison is base (4f8fe70) 51.41% compared to head (84e9c2c) 51.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1060      +/-   ##
==========================================
- Coverage   51.41%   51.37%   -0.05%     
==========================================
  Files          76       76              
  Lines        6595     6595              
==========================================
- Hits         3391     3388       -3     
- Misses       2938     2940       +2     
- Partials      266      267       +1     
Impacted Files Coverage Δ
controllers/workspace/status.go 75.58% <ø> (ø)
controllers/workspace/finalize.go 52.85% <40.00%> (ø)
controllers/workspace/devworkspace_controller.go 58.50% <100.00%> (-0.61%) ⬇️
pkg/library/lifecycle/poststart.go 95.16% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@amisevsk
Copy link
Collaborator Author

amisevsk commented Mar 3, 2023

ping @AObuchow waiting on approval

@AObuchow
Copy link
Collaborator

AObuchow commented Mar 3, 2023

ping @AObuchow waiting on approval

Going to approve this since everything worked with Che when I last tested this PR even when the tests were broken (since the tests weren't part of the actual DWO image)

@openshift-ci openshift-ci bot added the lgtm label Mar 3, 2023
@openshift-ci
Copy link

openshift-ci bot commented Mar 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@AObuchow AObuchow merged commit df84f45 into devfile:main Mar 3, 2023
@amisevsk amisevsk deleted the bump-controller-runtime branch March 3, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update controller-runtime to to v0.14.4
3 participants