-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Bump cert-manager and use their clientset #15703
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: skonto 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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15703 +/- ##
==========================================
- Coverage 80.85% 80.82% -0.04%
==========================================
Files 222 222
Lines 18035 18037 +2
==========================================
- Hits 14583 14578 -5
- Misses 3082 3088 +6
- Partials 370 371 +1 ☔ View full report in Codecov by Sentry. |
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect | ||
sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect | ||
) | ||
|
||
replace k8s.io/gengo/v2 v2.0.0-20240826214909-a7b603a56eb7 => k8s.io/gengo/v2 v2.0.0-20240228010128-51d4e06bde70 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this I am hitting kubernetes/code-generator#177 cc @dprotaso
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need the version in the first part of the replace directive
|
I will fix the lint thing although it is irrelevant to the PR it seems 🤔 |
Looking at the lint errors:
Probably we need to update the action to the latest because this seems like a false positive. Btw when we bump the linter version we will face issues like:
Unlike 1.62.2 1.63.4 does not like the directives there. cc @dprotaso |
463573f
to
82b465c
Compare
@dprotaso ready |
those seem like false positives (are they?) - my guess is they fixed a false positive in a newer version. I'd rather pick up the updated linter Can we bump the linter version in knative/actions? |
I bumped the linter here - knative/actions#243 |
@@ -369,7 +369,7 @@ func (c *Reconciler) clusterLocalDomainTLS(ctx context.Context, r *v1.Route, tc | |||
|
|||
orphanCerts, err := c.getOrphanRouteCerts(r, usedDomains, netcfg.CertificateClusterLocalDomain) | |||
if err != nil { | |||
return nil, nil | |||
return nil, nil //nolint:nilerr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you undo this nolint
directive. I left this unfixed in an earlier PR so I could double check why we're not handling this scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do an override to get passed this lint failure for this PR and deal with it in a follow up
Proposed Changes