-
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🏃 KCP cleanup #2303
🏃 KCP cleanup #2303
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha 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 |
cd6af6d
to
965da1c
Compare
/lgtm |
lgtm pending rebase after #2295 merges |
lgtm |
/hold cancel Rebased, much smaller diff now. |
/lgtm |
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" | ||
) | ||
|
||
// Log is the global logger for the internal package. | ||
var Log = klogr.New() |
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.
As we move forward we should figure out a better place for this. I think it's too early to bother figuring out right now, but I'm not sure if we want a logger per package or a logger per module.... 🤔 Will be paying attention as this pattern develops.
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.
Should this be exported?
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.
yeah, this should be exported, that way the other packages can control the logging for this library
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go
Outdated
Show resolved
Hide resolved
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" | ||
) | ||
|
||
// Log is the global logger for the internal package. | ||
var Log = klogr.New() |
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.
Should this be exported?
Signed-off-by: Chuck Ha <chuckh@vmware.com>
I think capd started flaking again. not sure why but that's why it's not required yet! 🙃 I have a perpetual note in my todo to look into it when i've got time |
@chuckha the hourlies aren't showing too many failures: https://k8s-testgrid.appspot.com/sig-cluster-lifecycle-cluster-api#capd%20e2e%20tests |
hmm I'll try rerunning until i finish up this other work and then can look into this /test pull-cluster-api-capd-e2e |
/lgtm |
/hold
This depends on #2295
What this PR does / why we need it:
This PR cleans up a few things from #2193. This will make the change to get the etcd health check even smaller and more integrated feeling.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Related to #2243
/assign @detiber @dlipovetsky @randomvariable
To expand a little bit on what's going on here: This is a minimal diff to start using the ManagementCluster and remove the functionality off the reconciler. Moving forward I'd like to take almost all of the logic in the reconciler and put it on the management cluster. This will keep the reconciler small and testable while focused on passing data to the right dependencies.