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 reconcilers and controller manager construction logic #674

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

chrisdoherty4
Copy link
Member

@chrisdoherty4 chrisdoherty4 commented Mar 19, 2023

These changes tidy up the tinker controller implementation in several ways:

  1. Reduce the manager to a single NewManager function with the workflow controller pre-registerred.
  2. Replace the full manager.Manager dependency in the Tink Server with a cluster.Cluster. Cluster objects are only really concerned with providing an API to retrieve a client and facilitate caching unlike the manager object that's concerned with all components related to controllers, webhooks, leader election and more. This makes the cluster object more appropriate for the Tink Server.
  3. Colocate used indexes in the packages that consume them and remove unused indexes. Historically the unused indexes have been consumed by other Tinkerbell project; the projects can copy the indexes if still needed.
  4. Rename workflow.Controller to workflow.Reconciler to be representative of the implementation. Controllers contain more than just reconciliation components.
  5. Patch through metrics and prob endpoint configuration to the controller manager. The CLI flags are the common flags exposed from controllers: --metrics-bind-address, --health-probe-bind-address.

@chrisdoherty4 chrisdoherty4 force-pushed the refactor/controllers branch 10 times, most recently from 39bcc7a to acdffd3 Compare March 20, 2023 01:04
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #674 (b9619ec) into main (5126bd0) will increase coverage by 1.03%.
The diff coverage is 42.59%.

❗ Current head b9619ec differs from pull request most recent head 9cba4cf. Consider uploading reports for the commit 9cba4cf to get more accurate results

@@            Coverage Diff             @@
##             main     #674      +/-   ##
==========================================
+ Coverage   48.26%   49.30%   +1.03%     
==========================================
  Files          18       18              
  Lines         951      860      -91     
==========================================
- Hits          459      424      -35     
+ Misses        484      428      -56     
  Partials        8        8              
Impacted Files Coverage Δ
internal/server/kubernetes_api.go 0.00% <0.00%> (ø)
internal/server/kubernetes_api_workflow.go 30.15% <0.00%> (ø)
internal/workflow/reconciler.go 79.31% <57.14%> (ø)
internal/server/index.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@chrisdoherty4 chrisdoherty4 force-pushed the refactor/controllers branch 3 times, most recently from 85f7c2a to 1b205bf Compare March 20, 2023 01:22
@chrisdoherty4 chrisdoherty4 marked this pull request as ready for review March 20, 2023 01:30
@chrisdoherty4 chrisdoherty4 changed the title Refactor reconcilers and controller manager Refactor reconcilers and controller manager construction logic Mar 20, 2023
@chrisdoherty4 chrisdoherty4 force-pushed the refactor/controllers branch 3 times, most recently from 6b8394d to 1455e4e Compare March 20, 2023 02:01
@chrisdoherty4 chrisdoherty4 marked this pull request as draft March 20, 2023 02:04
@chrisdoherty4 chrisdoherty4 force-pushed the refactor/controllers branch 4 times, most recently from 31d9a37 to c066851 Compare March 20, 2023 02:15
@chrisdoherty4 chrisdoherty4 marked this pull request as ready for review March 20, 2023 02:19
@chrisdoherty4 chrisdoherty4 added the ready-to-merge Signal to Mergify to merge the PR. label Mar 20, 2023
@mergify mergify bot merged commit 3bad8a6 into tinkerbell:main Mar 20, 2023
@chrisdoherty4 chrisdoherty4 deleted the refactor/controllers branch May 15, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants