Skip to content

Conversation

@ranakan19
Copy link
Contributor

@ranakan19 ranakan19 commented Mar 25, 2024

Updates k8s dependencies to 0.27 and controller-runtime to v0.15.
With the update to controller-runtime v0.15 (changes in release detailed here), this PR has following changes to combat the breaking changes:

Changes wrt - https://issues.redhat.com/browse/SANDBOX-559
DO NOT MERGE (All the PRs related to version updates need to be merged at once, but these PR are for early feedback so that there are not a lot of changes to review at once)

cl := http.Client{}

err := wait.Poll(DefaultRetryInterval, DefaultTimeout, func() (done bool, err error) {
err := wait.PollUntilContextTimeout(context.TODO(), DefaultRetryInterval, DefaultTimeout, false, func(ctx context.Context) (done bool, err error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Poll is being deprecated and the linter was complaining. However to fix this there was a typo in the function name that was suggested as replacement, which was later fixed.
PTAL here if the context is used correctly - I'm not a 100% sure.
For reference please check - https://github.com/kubernetes/apimachinery/issues/153 and the fix here - https://github.com/kubernetes/kubernetes/pull/117143/files

@codecov
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.17%. Comparing base (56327d5) to head (35f5fff).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...trollers/nstemplateset/nstemplateset_controller.go 0.00% 4 Missing ⚠️
controllers/useraccount/useraccount_controller.go 0.00% 3 Missing ⚠️
...roperatorconfig/memberoperatorconfig_controller.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   82.30%   82.17%   -0.13%     
==========================================
  Files          28       28              
  Lines        3243     3243              
==========================================
- Hits         2669     2665       -4     
- Misses        434      437       +3     
- Partials      140      141       +1     
Files with missing lines Coverage Δ
controllers/memberoperatorconfig/mapper.go 100.00% <100.00%> (ø)
...roperatorconfig/memberoperatorconfig_controller.go 75.00% <0.00%> (ø)
controllers/useraccount/useraccount_controller.go 82.17% <0.00%> (ø)
...trollers/nstemplateset/nstemplateset_controller.go 70.58% <0.00%> (-2.62%) ⬇️

@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
10.7% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

Looks good, but I have a few comments on the versions in go.mod file.

Also, is there a reason why you picked the v0.27.2 specifically, and not the latest v0.27.x patch version that is available at the time when we are doing the update? This is just a detail since we are going to bump the version to the next minor one soon again.

go.mod Outdated
k8s.io/klog v1.0.0
k8s.io/klog/v2 v2.70.1
k8s.io/klog/v2 v2.90.1
k8s.io/metrics v0.25.0
Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't this dependency k8s.io/metrics updated to v0.27.2 as well?

go.mod Outdated
github.com/prometheus/client_golang v1.15.1
k8s.io/apiextensions-apiserver v0.27.2
k8s.io/apimachinery v0.27.2
k8s.io/kubectl v0.24.0
Copy link
Contributor

Choose a reason for hiding this comment

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

the same - it should use

Suggested change
k8s.io/kubectl v0.24.0
k8s.io/kubectl v0.27.2

go.mod Outdated
github.com/go-logr/logr v1.2.3
github.com/go-logr/logr v1.2.4
github.com/google/go-cmp v0.5.9
// using latest commit from 'github.com/openshift/api branch release-4.12'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// using latest commit from 'github.com/openshift/api branch release-4.12'
// using latest commit from 'github.com/openshift/api branch release-4.14'

go.mod Outdated
github.com/openshift/api v0.0.0-20230213134911-7ba313770556
github.com/openshift/api v0.0.0-20231117205818-971e4ba78c9a
github.com/pkg/errors v0.9.1
github.com/redhat-cop/operator-utils v1.3.3-0.20220121120056-862ef22b8cdf
Copy link
Contributor

Choose a reason for hiding this comment

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

let's update this dependency as well to match the version of k8s v.0.27.x - it should be the version v1.3.6 https://github.com/redhat-cop/operator-utils/releases/tag/v1.3.6

@openshift-ci
Copy link

openshift-ci bot commented Aug 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, ranakan19

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:
  • OWNERS [alexeykazakov,ranakan19]

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

@sonarqubecloud

This comment was marked as outdated.

@sonarqubecloud
Copy link

@ranakan19 ranakan19 merged commit ca053bf into codeready-toolchain:master Nov 15, 2024
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants