Skip to content
This repository was archived by the owner on Jul 30, 2021. It is now read-only.

🏃‍♂️ Add a test to ensure that exactly one control plane machine initializes if there are multiple control plane machines defined #221

Merged
merged 1 commit into from
Sep 22, 2019
Merged

🏃‍♂️ Add a test to ensure that exactly one control plane machine initializes if there are multiple control plane machines defined #221

merged 1 commit into from
Sep 22, 2019

Conversation

SataQiu
Copy link
Contributor

@SataQiu SataQiu commented Sep 11, 2019

What this PR does / why we need it:
Add a test to ensure that exactly one control plane machine initializes if there are multiple control plane machines defined

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Ref #214

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 11, 2019
neolit123
neolit123 reviewed Sep 11, 2019
View reviewed changes
controllers/kubeadmconfig_controller_test.go
@@ -970,6 +970,67 @@ func TestKubeadmConfigReconciler_ClusterToKubeadmConfigs(t *testing.T) {
}
}

// Exactly one control plane machine initializes if there are multiple control plane machines defined
func TestKubeadmConfigReconciler_Reconcile_ExactlyOneControlPlaneMachineInitializes(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

side topic: i think that _ in function names fails golint
is this file ignored from golint checks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure actually..i didn't see anything in the code that indicated tests are skipped. I'd say let's not worry about it for this PR. I've made an issue for it #223

neolit123
neolit123 reviewed Sep 11, 2019
View reviewed changes
controllers/kubeadmconfig_controller_test.go Outdated
}
result, err := k.Reconcile(request)
if err != nil {
t.Fatal(fmt.Sprintf("Failed to reconcile:\n %+v", err))
Copy link
Contributor

@neolit123 neolit123 Sep 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Fatalf instead of Fatal + Sprintf
[1]

neolit123
neolit123 reviewed Sep 11, 2019
View reviewed changes
controllers/kubeadmconfig_controller_test.go Outdated
t.Fatal(fmt.Sprintf("Failed to reconcile:\n %+v", err))
}
if result.Requeue == true {
t.Fatal("did not expected to requeue")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did not expect

neolit123
neolit123 reviewed Sep 11, 2019
View reviewed changes
controllers/kubeadmconfig_controller_test.go Outdated
}
result, err = k.Reconcile(request)
if err != nil {
t.Fatal(fmt.Sprintf("Failed to reconcile:\n %+v", err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[1]

neolit123
neolit123 reviewed Sep 11, 2019
View reviewed changes
controllers/kubeadmconfig_controller_test.go Outdated
t.Fatal("did not expected to requeue")
}
if result.RequeueAfter != time.Duration(0) {
t.Fatal("did not expected to requeue after")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did not expect

neolit123
neolit123 reviewed Sep 11, 2019
View reviewed changes
controllers/kubeadmconfig_controller_test.go Outdated
t.Fatal(fmt.Sprintf("Failed to reconcile:\n %+v", err))
}
if result.Requeue == true {
t.Fatal("did not expected to requeue")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did not expect

neolit123
neolit123 reviewed Sep 11, 2019
View reviewed changes
controllers/kubeadmconfig_controller_test.go
m.locked = true
return true
}
return false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we always return true for the fake Lock?

@chuckha
Copy link
Contributor

chuckha commented Sep 11, 2019

This looks great to me pending @neolit123's changes.

The fake lock is a little bit weird, would it be possible to always return true? a simpler implementation would be better, but if this is necessary for now then let's leave it.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2019
@SataQiu
Copy link
Contributor Author

SataQiu commented Sep 16, 2019

This looks great to me pending @neolit123's changes.

The fake lock is a little bit weird, would it be possible to always return true? a simpler implementation would be better, but if this is necessary for now then let's leave it.

Thanks @chuckha
I don't think the fake lock should always return true.
Because only when the lock is unlocked can the initializing machine obtain the lock.

@chuckha
Copy link
Contributor

chuckha commented Sep 17, 2019

/approve

thanks for the update this looks really good!

/assign @neolit123
for lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2019
@chuckha chuckha changed the title Add a test to ensure that exactly one control plane machine initializes if there are multiple control plane machines defined 🏃‍♂️ Add a test to ensure that exactly one control plane machine initializes if there are multiple control plane machines defined Sep 17, 2019
@chuckha chuckha mentioned this pull request Sep 17, 2019
Closed
8 tasks
@SataQiu
Copy link
Contributor Author

SataQiu commented Sep 18, 2019

ping @neolit123

@SataQiu
add a test to ensure that exactly one control plane machine initializ…
279b3c3 
…es if there are multiple control plane machines defined
@SataQiu
Copy link
Contributor Author

SataQiu commented Sep 22, 2019

Hi @ncdc
Could you have a look at this?
This PR has been around for a long time

neolit123
neolit123 reviewed Sep 22, 2019
View reviewed changes
Copy link
Contributor

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the remarks i've made.
thanks.

@neolit123
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2019
@k8s-ci-robot k8s-ci-robot merged commit 92d49ae into kubernetes-retired:master Sep 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chuckha chuckha chuckha left review comments

@neolit123 neolit123 neolit123 left review comments

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

@ncdc ncdc Awaiting requested review from ncdc

Assignees

@neolit123 neolit123

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SataQiu @chuckha @k8s-ci-robot @neolit123