⚠️ Unexport MachineHealthCheck `patchUnhealthyTargets` method #4579

mshitrit · 2021-05-06T12:06:51Z

Tidy: changed empty slice declaration, modified var name that collides with pkg name, fixed typo and removed unused func variable

Signed-off-by: Michael Shitrit [email protected]

Not much just a bit of tidy up fixing couple of minor cosmetic stuff:

k8s-ci-robot · 2021-05-06T12:06:53Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

k8s-ci-robot · 2021-05-06T12:06:58Z

Hi @mshitrit. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

JoelSpeed

/ok-to-test

controllers/machinehealthcheck_controller.go

JoelSpeed · 2021-05-06T12:11:12Z

controllers/machinehealthcheck_controller.go

@@ -145,7 +145,7 @@ func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, req ctrl.R
 	defer func() {
 		// Always attempt to patch the object and status after each reconciliation.
 		// Patch ObservedGeneration only if the reconciliation completed successfully
-		patchOpts := []patch.Option{}
+		var patchOpts []patch.Option


Why is this preferred? These two lines are functionally equivalent, I think this is very much a style preference. Which is most used throughout the codebase?

I think that's the most common argumentation: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

Just saw that there is no actual explanation in there :)

According to go lint the second one is preferred because there is no memory allocation (just declaring a slice that is nil)

@mshitrit Did make lint fail on this?

I didn't try but I find it hard to believe that it will.
I caught it because of my golint plugin on my IDE - it's considered a "weak warning" which is pretty minor.

JoelSpeed · 2021-05-06T12:11:43Z

controllers/machinehealthcheck_controller.go

@@ -288,7 +288,7 @@ func (r *MachineHealthCheckReconciler) reconcile(ctx context.Context, logger log
 	conditions.MarkTrue(m, clusterv1.RemediationAllowedCondition)

 	errList := r.PatchUnhealthyTargets(ctx, logger, unhealthy, cluster, m)
-	errList = append(errList, r.PatchHealthyTargets(ctx, logger, healthy, cluster, m)...)
+	errList = append(errList, r.PatchHealthyTargets(ctx, logger, healthy, m)...)


Why was the cluster removed from this list in this change? Is the cluster not actually used in the PatchHealthyTargets?

Yep, exactly

vincepri

/hold

@mshitrit Can you please open issues before pull requests to explain why a change is needed? I'm not inclined to merge most of the proposed changes because these are personal preferences.

vincepri · 2021-05-06T15:29:18Z

controllers/machinehealthcheck_controller.go

-func (r *MachineHealthCheckReconciler) PatchHealthyTargets(ctx context.Context, logger logr.Logger, healthy []healthCheckTarget, cluster *clusterv1.Cluster, m *clusterv1.MachineHealthCheck) []error {
-	errList := []error{}
+// PatchHealthyTargets patches healthy machines with MachineHealthCheckSucceededCondition.
+func (r *MachineHealthCheckReconciler) PatchHealthyTargets(ctx context.Context, logger logr.Logger, healthy []healthCheckTarget, m *clusterv1.MachineHealthCheck) []error {


This is technically a breaking change because this method is exported (which probably shouldn't be)

@JoelSpeed Should we unexport this method and remove the Cluster parameter?

Seems this method was added as part of #3901, which also added PatchUnhealthyTargets. I think it's reasonable to unexport this, but in that case, I would suggest we do the same for PatchUnhealthyTargets too for consistency.

Seems the method has never used the cluster parameter so +1 to dropping that.

vincepri · 2021-05-06T15:31:17Z

controllers/machinehealthcheck_controller.go

@@ -75,7 +75,7 @@ type MachineHealthCheckReconciler struct {
 }

 func (r *MachineHealthCheckReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
-	controller, err := ctrl.NewControllerManagedBy(mgr).
+	managedController, err := ctrl.NewControllerManagedBy(mgr).


Please revert this.

Looking at the proposed changes, even though the variable collides with the package name, it reads much better as it is today. Moreover, it's consistent with the rest of the codebase and the controller package isn't used within this method.

mshitrit · 2021-05-06T16:36:15Z

Hi I've updated the PR.
Let me know if there are any other specific changes, or if you prefer I'll close the PR.
As I see it, it's a very minor cosmetic PR so either way is fine by me 😄
/cc @vincepri , @JoelSpeed @sbueringer

vincepri

/retitle ⚠️ Unexport MachineHealthCheck patchUnhealthyTargets method

vincepri · 2021-05-10T14:33:37Z

controllers/machinehealthcheck_controller.go

@@ -440,7 +440,7 @@ func (r *MachineHealthCheckReconciler) clusterToMachineHealthCheck(o client.Obje
 	}

 	// This list should only contain MachineHealthChecks which belong to the given Cluster
-	requests := []reconcile.Request{}
+	var requests []reconcile.Request


Let's revert these declarations? It's mostly a style preference (and a small saving in initial memory allocation, which is probably ok in most uses). I'd rather not set a precedent to merge changes to declarations for small gains.

Hmm I'm not sure I follow.
I think we can probable agree that var requests []reconcile.Request is better than requests := []reconcile.Request{} which is also supported by the official goland code standards.
I agree it's a minor change and I can understand the argument against merging a minor change, but since it's merged along with other changes and not all by itself I don't think this is a valid argument (unless we decide the whole PR is too minor to be merged).

I think we can probable agree that var requests []reconcile.Request is better than requests := []reconcile.Request{}

I'm trying to avoid these kind of discussions, which are based mostly on personal preferences.

but since it's merged along with other changes and not all by itself I don't think this is a valid argument

No. We make it an effort to keep the pull request focused on one task at hand. If all PRs start bringing in minor style changes, the noise to signal ratio becomes too high.

unless we decide the whole PR is too minor to be merged

I'm fine merging the unexporting of the method, and removing the unused parameter. The rest of the changes (minor or not) are personal preferences that I'm not comfortable merging, even if they were standing in a different PR on their own.

I guess we'll have to agree to disagree on that.
But since I'm a new guest to this repo I think it's only common curtsy to respect the house rules 🙂

Signed-off-by: Michael Shitrit <[email protected]>

k8s-ci-robot · 2021-05-12T06:35:45Z

@mshitrit: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`ae8da03`	link	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

fabriziopandini · 2021-05-12T08:16:04Z

/hold cancel
as per comments above

/lgtm

sbueringer · 2021-05-13T18:47:59Z

/lgtm

mshitrit · 2021-05-18T11:32:59Z

Hi All,
IIUC the merge is pending due to some sort of validation that fails because of the API change.
I'm not really sure what's the proper procedure in this case - please advise.

vincepri

/approve

k8s-ci-robot · 2021-05-18T14:22:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

~~OWNERS~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sbueringer · 2021-05-18T16:53:50Z

@mshitrit Just fyi pull-cluster-api-apidiff-main is a non-blocking test (which is now obvious I guess :))

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 6, 2021

k8s-ci-robot requested review from JoelSpeed and sbueringer May 6, 2021 12:06

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 6, 2021

JoelSpeed reviewed May 6, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 6, 2021

vincepri requested changes May 6, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2021

mshitrit force-pushed the tidy branch from 21b3046 to 37fbe8d Compare May 6, 2021 16:21

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 6, 2021

mshitrit force-pushed the tidy branch 2 times, most recently from c9b1b4b to 906058b Compare May 9, 2021 07:48

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 9, 2021

mshitrit force-pushed the tidy branch from 906058b to 3930956 Compare May 9, 2021 07:49

vincepri requested changes May 10, 2021

View reviewed changes

k8s-ci-robot changed the title ~~🌱 Tidy: changed empty slice declaration, modified var name that collide…~~ ⚠️ Unexport MachineHealthCheck patchUnhealthyTargets method May 10, 2021

Tidy: fixed typo, unexported methods and removed unused func variable

ae8da03

Signed-off-by: Michael Shitrit <[email protected]>

mshitrit force-pushed the tidy branch from 3930956 to ae8da03 Compare May 12, 2021 06:34

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 12, 2021

k8s-ci-robot assigned fabriziopandini May 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2021

k8s-ci-robot assigned sbueringer May 13, 2021

vincepri approved these changes May 18, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2021

k8s-ci-robot merged commit 97f658d into kubernetes-sigs:master May 18, 2021

k8s-ci-robot added this to the v0.4 milestone May 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Unexport MachineHealthCheck `patchUnhealthyTargets` method #4579

⚠️ Unexport MachineHealthCheck `patchUnhealthyTargets` method #4579

mshitrit commented May 6, 2021

k8s-ci-robot commented May 6, 2021

k8s-ci-robot commented May 6, 2021

JoelSpeed left a comment

JoelSpeed May 6, 2021

sbueringer May 6, 2021 •

edited

Loading

mshitrit May 6, 2021

vincepri May 10, 2021

mshitrit May 11, 2021

JoelSpeed May 6, 2021

mshitrit May 6, 2021 •

edited

Loading

vincepri left a comment

vincepri May 6, 2021

vincepri May 6, 2021

JoelSpeed May 6, 2021

vincepri May 6, 2021

mshitrit commented May 6, 2021

vincepri left a comment

vincepri May 10, 2021

mshitrit May 11, 2021

vincepri May 11, 2021 •

edited

Loading

mshitrit May 12, 2021

k8s-ci-robot commented May 12, 2021

fabriziopandini commented May 12, 2021

sbueringer commented May 13, 2021

mshitrit commented May 18, 2021

vincepri left a comment

k8s-ci-robot commented May 18, 2021

sbueringer commented May 18, 2021

⚠️ Unexport MachineHealthCheck patchUnhealthyTargets method #4579

⚠️ Unexport MachineHealthCheck patchUnhealthyTargets method #4579

Conversation

mshitrit commented May 6, 2021

k8s-ci-robot commented May 6, 2021

k8s-ci-robot commented May 6, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshitrit May 6, 2021 • edited Loading

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshitrit commented May 6, 2021

vincepri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 12, 2021

fabriziopandini commented May 12, 2021

sbueringer commented May 13, 2021

mshitrit commented May 18, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 18, 2021

sbueringer commented May 18, 2021

⚠️ Unexport MachineHealthCheck `patchUnhealthyTargets` method #4579

⚠️ Unexport MachineHealthCheck `patchUnhealthyTargets` method #4579

sbueringer May 6, 2021 •

edited

Loading

mshitrit May 6, 2021 •

edited

Loading

vincepri May 11, 2021 •

edited

Loading