-
Notifications
You must be signed in to change notification settings - Fork 35
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
Simplify data structures and remove redundant interfaces #305
Simplify data structures and remove redundant interfaces #305
Conversation
Signed-off-by: Chris Doherty <chris.doherty4@gmail.com>
6eb5e3e
to
7f1a757
Compare
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.
The functionality in this file is 90% copy-and-paste from the previous machine.go and base.go files. Lines 705 to 1046 are copied from base.go. Everything else is the original machine.go.
patchHelper *patch.Helper | ||
client client.Client | ||
|
||
machine *clusterv1.Machine |
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.
These 3 fields were moved from the base object to ensure the existing functions could work without alteration.
0746934
to
722b63a
Compare
@@ -54,30 +56,79 @@ type TinkerbellMachineReconciler struct { | |||
// +kubebuilder:rbac:groups=bmc.tinkerbell.org,resources=jobs,verbs=get;list;watch;create | |||
|
|||
// Reconcile ensures that all Tinkerbell machines are aligned with a given spec. | |||
func (tmr *TinkerbellMachineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | |||
bmrc, err := tmr.newReconcileContext(ctx, req.NamespacedName) |
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.
The implementation of the newReconcileContext()
has been moved to the Reconcile()
call (lines 64-91).
} | ||
|
||
mrc, err := bmrc.IntoMachineReconcileContext() |
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.
The implementation of IntoMachineReconcileContext()
has been moved to the Reconcile()
call (lines 97-131).
t.Run("reconciler_is_nil", machineReconciliationPanicsWhenReconcilerIsNil) //nolint:paralleltest | ||
t.Run("reconciler_has_no_client_set", machineReconciliationPanicsWhenReconcilerHasNoClientSet) //nolint:paralleltest |
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.
See inline comments for explanation on the panic (tinkerbellmachine_controller.go:60)
The base and machine reconciliation contexts were confusing and indicative of a hierarchical relationship. This change simplifies the 2 data structures into a single machineReconcileScope data structure. The root TinkerbellMachineController is now responsible for correctly constructing the scope object before calling its Reconcile() function. This reduces the reconciliation complexity. Signed-off-by: Chris Doherty <chris.doherty4@gmail.com>
722b63a
to
0d6e997
Compare
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at bc4a272 |
Summary
The existing machine reconciliation code leveraged 3 different data structures (
TinkerbellMachineReconciler
,machineReconcileContext
andbaseMachineReconcileContext
) and 2 interfaces (ReconcileContext
andBaseMachineReconcileContext
).The
machineReconcileContext
extends thebaseMachineReconcileContext
but the relationship is misappropriated and superfluous.The
ReconcileContext
and theBaseMachineReconcileContext
have no discernible purpose.Changes
ReconcileContext
andBaseMachineReconcileContext
interfaces.baseMachineReconcileContext
andmachineReconcileContext
into a single data structure,machineReconcileScope
.machineReconcileScope
construction logic to theTinkerbellMachineReconciler.Reconcile()
call keeping the construction in a single place.Motivation
What's next?