Skip to content

Conversation

@erikgb
Copy link
Member

@erikgb erikgb commented May 30, 2025

The field Result.Requeue is deprecated in controller-runtime 0.21.0, and I don't know why we are using it at all - since the Godoc of Result.RequeueAfter states:

// RequeueAfter if greater than 0, tells the Controller to requeue the reconcile key after the Duration.
// Implies that Requeue is true, there is no need to set Requeue to true at the same time as RequeueAfter.

Blocks #640.

@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 30, 2025
@erikgb erikgb requested a review from wallrj May 30, 2025 09:25
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @erikgb

I see that this was flagged by the verify checks in #640 and that this will allow us to upgrade to the latest version of controller-runtime. Good.

pkg/internal/controllers/certificaterequestpolicies.go:198:6: SA1019: response.Requeue is deprecated: Use `RequeueAfter` instead. (staticcheck)
		if response.Requeue || response.RequeueAfter > 0 {
		   ^
pkg/internal/controllers/certificaterequestpolicies.go:199:8: SA1019: result.Requeue is deprecated: Use `RequeueAfter` instead. (staticcheck)
			if !result.Requeue || result.RequeueAfter > response.RequeueAfter {
			    ^
pkg/internal/controllers/certificaterequestpolicies.go:202:4: SA1019: result.Requeue is deprecated: Use `RequeueAfter` instead. (staticcheck)
			result.Requeue = true
			^
3 issues:

FTR: I agree with some of the comments in kubernetes-sigs/controller-runtime#3107. I do think it was a useful option to be able to requeue and make use of the builtin backoff timer...but I'm not sure if that how it was intended to be used here.

/approve
/lgtm

@cert-manager-prow cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2025
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

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

@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2025
@cert-manager-prow cert-manager-prow bot merged commit 9b2d4ad into cert-manager:main May 30, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants