WIP - use logger from ctx instead of scope #1861
Conversation
k8s-ci-robot
added
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
cncf-cla: yes
|
Before doing other files, I want to see the impact on the openstackcluster controller logs and if that changed anything. |
|
/test pull-cluster-api-provider-openstack-e2e-test |
| @@ -491,7 +496,7 @@ func reconcileNetworkComponents(scope scope.Scope, cluster *clusterv1.Cluster, o | |||
| } | |||
|
|
|||
| if openStackCluster.Spec.NodeCIDR == "" { | |||
| scope.Logger().V(4).Info("No need to reconcile network, searching network and subnet instead") | |||
| log.Info("No need to reconcile network, searching network and subnet instead") | |||
There was a problem hiding this comment.
Caution when you're making all these mechanical changes: please don't lose the .V(n) bit.
| log.Info("No need to reconcile network, searching network and subnet instead") | |
| log.V(4).Info("No need to reconcile network, searching network and subnet instead") |
k8s-ci-robot
added
size/L
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: EmilienM, huxcrux The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-rebase
|
@EmilienM you mentioned you weren't happy with how invasive this change became. Based on that feedback I created #1913, which is less invasive because it doesn't require changing the Logger() calls. Do you want to close this PR? I feel like we're likely to end up touching all of this stuff anyway, e.g. to pass Context everywhere in Gophercloud v2. I also think this highlights a design smell in Scope (logger probably shouldn't be in there). Still, I'll be glad not to do that work today. |
|
/close |
|
@EmilienM: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
Wwe create a cached provider scope and we put the logger in it. This has caused issue #1840 so now we'll get the logger from context.
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 #1840
TODOs:
/hold