🏃 Push logger further down into KubeadmControlPlane reconciliation methods #2380
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber 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 |
k8s-ci-robot
added
approved
|
/assign @vincepri @chuckha @dlipovetsky |
|
/test pull-cluster-api-capd-e2e |
There was a problem hiding this comment.
This PR is doing a bit more than just pushing the logger deeper, it appears to be a number of various clean ups.
However, the only question is around logging context and a global logger. If we need some context we cannot or do not want to rebuild deeper in the reconcile function, then passing the logger through makes sense. Otherwise, we can avoid passing the logger around and expanding a number of parameter lists by using a package (or module) global logger.
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go
Outdated
Show resolved
Hide resolved
|
Oh, i see what's happening. I only need to be reviewing a single commit, not both. |
ah, yes, sorry. Things are a bit intertwined and this was my attempt to try and make it easier to review as well as start landing some of the changes independent of the full functionality |
|
no worries, a comment in the description can help, but it's not a huge deal |
|
/milestone v0.3.0-rc.2 |
ncdc
added
the
area/control-plane
detiber
force-pushed
the
kcpPushLoggerDown
branch
2 times, most recently
from
298697f
to
b227c77
Compare
detiber
force-pushed
the
kcpPushLoggerDown
branch
from
b227c77
to
4852f4d
Compare
| func (r *KubeadmControlPlaneReconciler) upgradeControlPlane(ctx context.Context, cluster *clusterv1.Cluster, kcp *controlplanev1.KubeadmControlPlane, requireUpgrade internal.FilterableMachineCollection) error { | ||
|
|
||
| func (r *KubeadmControlPlaneReconciler) upgradeControlPlane(_ context.Context, cluster *clusterv1.Cluster, kcp *controlplanev1.KubeadmControlPlane) (ctrl.Result, error) { //nolint | ||
| _ = r.Log.WithValues("namespace", kcp.Namespace, "kubeadmControlPlane", kcp.Name, "cluster", cluster.Name) |
There was a problem hiding this comment.
presumably this is for the following commits?
There was a problem hiding this comment.
yeah, mostly so I don't forget when I'm in refactor hell.
k8s-ci-robot
added
the
lgtm
What this PR does / why we need it:
Pushes the logger further down into the KubeadmControlPlane reconciliation methods to clean up the logging and error handling.
Related to #2242
Builds on #2379