-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃尡 ClusterClass: fix some nits in the topology reconciler #5759
馃尡 ClusterClass: fix some nits in the topology reconciler #5759
Conversation
@@ -117,6 +117,13 @@ func (r *ClusterReconciler) getCurrentControlPlaneState(ctx context.Context, clu | |||
if err != nil { | |||
return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate for %s", tlog.KObj{Obj: res.Object}) | |||
} | |||
// FIXME(TBD) should we do this? If yes, we should do the same for the MD templates |
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.
TBD: I saw that we check for the label on the InfraCluster and ControlPlane. Do we want to do this for the other templates too?
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.
I don't have objections, but let's move to a separate issue given that this is not a nit?
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.
Good point, removed from this PR
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.
@@ -265,11 +265,3 @@ func isOwnerReferenceEqual(a, b metav1.OwnerReference) bool { | |||
return true | |||
} | |||
|
|||
func ownerRefereceTo(obj client.Object) metav1.OwnerReference { |
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.
Turns out we had two of those :)
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.
I remember this. I think it was because the 2 PRs that were adding this functions were in pipeline at the same time. :)
f08815a
to
656ab3f
Compare
@@ -269,15 +269,15 @@ func (r *ClusterReconciler) reconcileMachineDeployments(ctx context.Context, s * | |||
func (r *ClusterReconciler) createMachineDeployment(ctx context.Context, md *scope.MachineDeploymentState) error { | |||
log := tlog.LoggerFrom(ctx).WithMachineDeployment(md.Object) | |||
|
|||
ctx, _ = log.WithObject(md.InfrastructureMachineTemplate).Into(ctx) | |||
if err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{ | |||
infraCtx, _ := log.WithObject(md.InfrastructureMachineTemplate).Into(ctx) |
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.
I'm fine with keeping the old version, but it just seemed not ideal to me to stack loggers in the context on the "same level" (I realize it's a regular pattern when traversing deeper into a call stack).
With the previous code the context would have:
- parent loggers
- a logger for infra machine
- a logger for bootstrap
It's not really a problem as the last one is used.
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.
lgtm pending split of label check into a separated issue
@@ -117,6 +117,13 @@ func (r *ClusterReconciler) getCurrentControlPlaneState(ctx context.Context, clu | |||
if err != nil { | |||
return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate for %s", tlog.KObj{Obj: res.Object}) | |||
} | |||
// FIXME(TBD) should we do this? If yes, we should do the same for the MD templates |
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.
I don't have objections, but let's move to a separate issue given that this is not a nit?
656ab3f
to
821a3ec
Compare
LGTM pending lint fix. @sbueringer Thanks for fixing my typos 馃槄. |
Signed-off-by: Stefan B眉ringer buringerst@vmware.com
821a3ec
to
8e22a8e
Compare
Ups, lint issue fixed. Thx! I'm still using Intellij with an active spell checker (with a large dictionary of custom added words) so sooner or later I'm picking up most of them :) |
/lgtm |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
Signed-off-by: Stefan B眉ringer buringerst@vmware.com
What this PR does / why we need it:
Just fixes some nits in the Cluster topology implementation (Nope I didn't spellcheck manually :), I just ran Intellij / Analyze over the whole package)
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #