Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add dynamic cert-manager certificates informer #8517

Merged
merged 4 commits into from
Mar 10, 2025
Merged

Add dynamic cert-manager certificates informer #8517

merged 4 commits into from
Mar 10, 2025
+159 −63

Conversation

matzew
Copy link
Member

@matzew matzew commented Mar 6, 2025

Fixes #

Proposed Changes

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note

Docs

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 6, 2025
@knative-prow knative-prow bot requested review from aslom and Cali0707 March 6, 2025 11:56
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2025
matzew
matzew commented Mar 6, 2025
View reviewed changes
pkg/reconciler/integration/sink/integrationsink.go Outdated
cert, err := r.cmCertificateLister.Certificates(sink.Namespace).Get(expected.Name)
lister := r.cmCertificateLister.Load()
if lister == nil {
return nil, fmt.Errorf("no cer-manager certificate lister created yet, this should never happen")
Copy link
Member Author

Choose a reason for hiding this comment

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

hitting this.

@matzew
Copy link
Member Author

matzew commented Mar 6, 2025

Do not merge, will investigate on the failures

@codecov Codecov
Copy link

codecov bot commented Mar 6, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 9.61538% with 94 lines in your changes missing coverage. Please review.

Project coverage is 52.39%. Comparing base (7be5c19) to head (c08941a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/certificates/certificate.go 0.00% 61 Missing ⚠️
pkg/reconciler/integration/sink/controller.go 0.00% 13 Missing ⚠️
pkg/reconciler/eventtransform/controller.go 0.00% 9 Missing ⚠️
pkg/reconciler/eventtransform/eventtransform.go 62.50% 4 Missing and 2 partials ⚠️
pkg/reconciler/integration/sink/integrationsink.go 0.00% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8517      +/-   ##
==========================================
- Coverage   52.50%   52.39%   -0.11%     
==========================================
  Files         397      397              
  Lines       24778    24836      +58     
==========================================
+ Hits        13010    13014       +4     
- Misses      10982    11036      +54     
  Partials      786      786

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@matzew @pierDipi
Add dynamic informer, part I
376fd82 
Signed-off-by: Matthias Wessendorf <[email protected]>
@pierDipi pierDipi changed the title WIP: Add dynamic informer, part I WIP: Add dynamic cert-manager certificates informer Mar 10, 2025
@pierDipi
Copy link
Member

Now it works for both IntegrationSink and EventTransform and manually tested a few transitions disabled -> permissive -> strict -> disabled

@pierDipi
Fix error message
c08941a 
Signed-off-by: Pierangelo Di Pilato <[email protected]>
@matzew matzew changed the title WIP: Add dynamic cert-manager certificates informer Add dynamic cert-manager certificates informer Mar 10, 2025
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2025
@matzew
Copy link
Member Author

matzew commented Mar 10, 2025

Now it works for both IntegrationSink and EventTransform and manually tested a few transitions disabled -> permissive -> strict -> disabled

same

pierDipi
pierDipi approved these changes Mar 10, 2025
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2025
@knative-prow Knative Prow
Copy link

knative-prow bot commented Mar 10, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, pierDipi

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

@matzew
Copy link
Member Author

matzew commented Mar 10, 2025

/retest

@knative-prow knative-prow bot merged commit ebd149e into knative:main Mar 10, 2025
33 of 36 checks passed
@matzew matzew deleted the dynamic_informer branch March 11, 2025 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi approved these changes

@aslom aslom Awaiting requested review from aslom

@Cali0707 Cali0707 Awaiting requested review from Cali0707

Assignees

@pierDipi pierDipi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matzew @pierDipi