🏃Added Test :Return early if the owning machine does not have an associated cluster #240

hpandeycodeit · 2019-09-20T20:17:16Z

What this PR does / why we need it:
This PR adds a test case - Return early if the owning machine does not have an associated cluster.

Which issue(s) this PR fixes :
Ref #214

k8s-ci-robot · 2019-09-20T20:17:17Z

Welcome @hpandeycodeit!

It looks like this is your first PR to kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

detiber · 2019-09-20T20:26:24Z

controllers/kubeadmconfig_controller_test.go

@@ -192,6 +192,34 @@ func TestKubeadmConfigReconciler_Reconcile_ReturnEarlyIfMachineHasBootstrapData(
 	}
 }

+// Return early If the owning machine does not have a associated cluster
+func TestKubeadmConfigReconciler_Reconcile_ReturnEarlyIfMachineHasNoCluster(t *testing.T) {


@chuckha @ncdc based on the discussions from the f2f planning, is this the behavior that we actually want? Or do we want to allow a Machine without a Cluster as long as the label isn't set and have some fallback behavior to just not provide the defaults we set from the Cluster resource when that is the case?

This is for the use-case that only uses the Machine API?

@chuckha indeed

My understanding was that a Cluster object was still required even if it's an empty object. It's good to figure this out now though. Is that also your understanding?

That is the direction that @ncdc, @vincepri, and myself have been advocating for, but I believe that the Cluster has been specified to still be optional at this point in time.

Cluster is required as of today. This is not a change in behavior but a formalization of the current behavior. If we can open an issue to get the Machine API use-case here then we can work on iterating from this place:

CABPK requires a Cluster
Cluster API requires a Cluster
CAPA requires a Cluster

SataQiu

found small nit

SataQiu · 2019-09-23T06:51:39Z

controllers/kubeadmconfig_controller_test.go

@@ -192,6 +192,34 @@ func TestKubeadmConfigReconciler_Reconcile_ReturnEarlyIfMachineHasBootstrapData(
 	}
 }

+// Return early If the owning machine does not have a associated cluster


Suggested change

// Return early If the owning machine does not have a associated cluster

// Return early If the owning machine does not have an associated cluster.

chuckha

one minor change and this looks good to me!

chuckha · 2019-09-23T18:53:02Z

controllers/kubeadmconfig_controller_test.go

+		},
+	}
+	_, err := k.Reconcile(request)
+	if err == nil {


I believe this recently changed, If the cluster does not exist then we expect error to be nil as it will simply be reconciled again. If you flip the == to be != and fix up the t.Fatal call then this looks good to me

chuckha · 2019-09-23T20:56:46Z

can you please squash your commits?

chuckha · 2019-09-23T20:57:45Z

controllers/kubeadmconfig_controller_test.go

+	}
+	_, err := k.Reconcile(request)
+	if err != nil {
+		t.Fatal("Not Expecting error, got an error")


It would be good to also print out the error, something like t.Fatalf("Not Expecting error, got an error: %+v", err)

thanks for pointing it out! Added the error in the t.fatalf

…an associated cluster

chuckha · 2019-09-23T21:12:12Z

/lgtm

chuckha · 2019-09-23T21:12:26Z

/approve

k8s-ci-robot · 2019-09-23T21:12:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, hpandeycodeit

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~~ [chuckha]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 20, 2019

k8s-ci-robot requested review from chuckha and SataQiu September 20, 2019 20:17

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 20, 2019

detiber reviewed Sep 20, 2019

View reviewed changes

SataQiu reviewed Sep 23, 2019

View reviewed changes

chuckha reviewed Sep 23, 2019

View reviewed changes

chuckha mentioned this pull request Sep 23, 2019

Testing for expected behaviors #214

Closed

8 tasks

chuckha reviewed Sep 23, 2019

View reviewed changes

hpandeycodeit force-pushed the master branch from a31db75 to be00257 Compare September 23, 2019 20:58

Added test case for Return early if the owning machine does not have …

2b93a31

…an associated cluster

hpandeycodeit force-pushed the master branch from be00257 to 2b93a31 Compare September 23, 2019 21:04

k8s-ci-robot assigned chuckha Sep 23, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2019

k8s-ci-robot merged commit 7aea998 into kubernetes-retired:master Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏃Added Test :Return early if the owning machine does not have an associated cluster #240

🏃Added Test :Return early if the owning machine does not have an associated cluster #240

hpandeycodeit commented Sep 20, 2019

k8s-ci-robot commented Sep 20, 2019

detiber Sep 20, 2019

chuckha Sep 23, 2019

detiber Sep 23, 2019

chuckha Sep 23, 2019

detiber Sep 23, 2019

chuckha Sep 23, 2019

SataQiu left a comment

SataQiu Sep 23, 2019

hpandeycodeit Sep 23, 2019

chuckha left a comment

chuckha Sep 23, 2019

hpandeycodeit Sep 23, 2019

chuckha commented Sep 23, 2019

chuckha Sep 23, 2019

hpandeycodeit Sep 23, 2019

chuckha commented Sep 23, 2019

chuckha commented Sep 23, 2019

k8s-ci-robot commented Sep 23, 2019

	// Return early If the owning machine does not have a associated cluster
	// Return early If the owning machine does not have an associated cluster.

🏃Added Test :Return early if the owning machine does not have an associated cluster #240

🏃Added Test :Return early if the owning machine does not have an associated cluster #240

Conversation

hpandeycodeit commented Sep 20, 2019

k8s-ci-robot commented Sep 20, 2019

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

Choose a reason for hiding this comment

SataQiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha commented Sep 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha commented Sep 23, 2019

chuckha commented Sep 23, 2019

k8s-ci-robot commented Sep 23, 2019