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.

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

Refactor watchers - Create separate type for namespaced informers #3238

Merged
merged 4 commits into from Nov 14, 2022

Conversation

ciarams87
Copy link
Member

Proposed changes

Refactor namespaced watchers. This change creates a new namespacedInformer types in controller.go, and in the certmanager and externaldns controllers.

This simplifies the logic a bit, and allows breaking up larger methods into smaller methods/ functions.
Additionally, it also provides a base for creating new or deleting unused namespacedInformers during runtime, which could allow future dynamic updating of which namespaces are watched.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@ciarams87 ciarams87 added the chore Pull requests for routine tasks label Nov 10, 2022
@github-actions github-actions bot removed the chore Pull requests for routine tasks label Nov 10, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2022

Codecov Report

Merging #3238 (10a5619) into main (42dd0d1) will increase coverage by 0.18%.
The diff coverage is 18.09%.

❗ Current head 10a5619 differs from pull request most recent head 7ec801c. Consider uploading reports for the commit 7ec801c to get more accurate results

@@            Coverage Diff             @@
##             main    #3238      +/-   ##
==========================================
+ Coverage   52.48%   52.67%   +0.18%     
==========================================
  Files          59       59              
  Lines       16104    16107       +3     
==========================================
+ Hits         8452     8484      +32     
+ Misses       7372     7341      -31     
- Partials      280      282       +2     
Impacted Files Coverage Δ
internal/k8s/controller.go 11.27% <4.80%> (+0.32%) ⬆️
internal/externaldns/controller.go 6.20% <15.68%> (+6.20%) ⬆️
internal/certmanager/cm_controller.go 15.26% <35.89%> (-2.78%) ⬇️
internal/k8s/status.go 34.51% <43.58%> (+1.24%) ⬆️
internal/certmanager/helper.go 87.50% <53.33%> (-12.50%) ⬇️
internal/certmanager/sync.go 72.95% <100.00%> (-0.28%) ⬇️
internal/externaldns/sync.go 38.37% <100.00%> (+1.42%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ciarams87 ciarams87 marked this pull request as ready for review November 11, 2022 14:17
@ciarams87 ciarams87 requested a review from a team as a code owner November 11, 2022 14:17
Copy link
Contributor

@shaun-nx shaun-nx left a comment

Choose a reason for hiding this comment

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

🚀

@ciarams87 ciarams87 merged commit 52ec611 into main Nov 14, 2022
@ciarams87 ciarams87 deleted the refactos-watchers branch November 14, 2022 13:17
@ciarams87 ciarams87 added the chore Pull requests for routine tasks label Nov 15, 2022
coolbry95 pushed a commit to coolbry95/kubernetes-ingress that referenced this pull request Nov 18, 2022
…inxinc#3238)

* Create separate type for namespaced watchers

* Create separate type for namespaced watchers in certmanager controller

* Create separate type for namespaced watchers in extdns controller

* Fix case where namespace is not watching secrets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants