🌱Added function to update control plane endpoint #1004

abdurrehman107 · 2023-10-10T12:35:12Z

What this PR does / why we need it:
This one fixes a part of the issue #977. Added utiltiy function to update control plane endpoints.
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 #977

Special notes for your reviewer:
This is not the complete PR. I'll add the unit tests. I request to get my function reviewed. I want to know what I can make better with my function. I'm currently feeling a little unsure about the false condition that I've added to the end of the function which can potentially set the status of the hetzner cluster to false. Is it okay if it stays that way? Or do I edit it up? I'd love feedback.

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
No it does not
TODOs:

squash commits
include documentation
add unit tests

controllers/hetznercluster_controller.go

abdurrehman107 · 2023-10-12T06:31:26Z

Hi @janiskemper . Okay basic question. I have worked with a few unit tests earlier however they were for relatively simpler Go code probably hence they contained functions such as func Testxxx(). Now going through hetznercluster_controller_tests.go its a little different. I've tried going through the test file and I'm still understanding things, but can you maybe just point me to the right direction with where do I start.
Like do I just create a function like func TestSetControlPlaneEndpoint() and maybe proceed to write code there? I'm sorry I just feel a little lost and I feel a little direction would really help.

janiskemper · 2023-10-12T06:33:11Z

that's exactly why I said

I would use the standard Go library for the unit tests of this function, instead of Gingko/gomega we use with envtest

yes you can and should proceed like that

abdurrehman107 · 2023-10-12T06:35:04Z

Oh okay okay. Aight. I'll proceed this way. Thank you Janis

abdurrehman107 · 2023-10-12T10:23:19Z

Hey @janiskemper can you please go through the below code?
So I'm basically trying to pass the hetznercluster as input to the function, now what I'm not able to understand is how do I pass it as an argument in the code below? I'm sure there is something very basic I'm missing. I'd love if you can help out.

tests := []struct{
name string
args args
want bool
}{
{
name: "should return true",
args: args{hetznerCluster},
want: true,
},
{
name: "should return false",
args: args{hetznerCluster},
want: false,
},
}

janiskemper · 2023-10-12T10:26:40Z

I'm not sure what "args" is in your code...

abdurrehman107 · 2023-10-12T11:22:06Z

Oh I'm sorry. I had created a type which includes the hetznercluster. Wait I'll put in the code: (cc @janiskemper)
Is this approach right? I'm trying to figure out how do I pass an object of hetznercluster in this.

type args struct {
hetznerCluster *infrav1.HetznerCluster
}
tests := []struct{
name string
args args
want bool
}{
{
name: "should return true",
args: args{hetznerCluster},
want: true,
},
{
name: "should return false",
args: args{hetznerCluster},
want: false,
},
}

abdurrehman107 · 2023-10-12T13:44:50Z

Hey @janiskemper . I'd really appreciate if you can provide feedback on the latest commit. As of now I've kept the two test cases almost same, I understand I will have to make changes to them. But I feel a little lost as to how do I make updates with the hetznercluster parameter. Like how do I set a case in which the test returns false, and how do I set a case in which a test returns true, this is what is confusing me. I hope I'm able to convey my point.

I also hope I am going in the right direction. I'd really appreciate your feedback.

janiskemper · 2023-10-12T14:12:26Z

From the code side you have created a pattern that you can use to write tests. But I would say that you struggle right now what you should test..
Well, pretty much tests that cover most of or even all of the logic that the function contains.

According to the pattern
if I do xyz, then I expect abc.
If i do xyz2, then I expect abc2.

One shortcoming of your current approach is that not only it is interesting which boolean the function returns, but also the changes to the hetznerCluster have to be validated.

I suggest that you first start with a list of tests that you wanna implement... Not writing code yet, just what would make sense to do.

abdurrehman107 · 2023-10-12T15:51:09Z

func SetControlPlaneEndpoint(hetznerCluster *infrav1.HetznerCluster) (bool) {
	if hetznerCluster.Spec.ControlPlaneLoadBalancer.Enabled {
		if hetznerCluster.Status.ControlPlaneLoadBalancer.IPv4 != "<nil>" {
			defaultHost := hetznerCluster.Status.ControlPlaneLoadBalancer.IPv4
			defaultPort := int32(hetznerCluster.Spec.ControlPlaneLoadBalancer.Port)

			if hetznerCluster.Spec.ControlPlaneEndpoint == nil {
				hetznerCluster.Spec.ControlPlaneEndpoint = &clusterv1.APIEndpoint{
					Host: defaultHost,
					Port: defaultPort,
				}
			} else {
				if hetznerCluster.Spec.ControlPlaneEndpoint.Host == "" {
					hetznerCluster.Spec.ControlPlaneEndpoint.Host = defaultHost
				}
				if hetznerCluster.Spec.ControlPlaneEndpoint.Port == 0 {
					hetznerCluster.Spec.ControlPlaneEndpoint.Port = defaultPort
				}
			}
			return true
		}
	} else if hetznerCluster.Spec.ControlPlaneEndpoint != nil {
		return true
	}
	return false
}

Okay so according to the code above I feel we'd want a few specefic test cases to take place which return true:

So if the Loadbalancer is enabled and its IPV4 holds a string value of "nil" then the test should by default just return true (a few things will get set in the if condition, but they do not matter to us, what matters is the return value which will be true).
Otherwise if that is not true and if the ControlPlaneEndpoint is nil then we should also return true (just that this time we did not set the values for other things as we did in the previous if statement)

If none of these match then the function should return false.

Seeing the above conditions, I think I should include three test cases. However, the problem still persists, how should I mention these conditions when writing the unit test? I dont think its supposed to be mentioned in the tests struct or is it?

janiskemper · 2023-10-12T18:21:46Z

I believe that there are a lot more possible tests. Pretty much every if-else statement gives you another possibility to test something

abdurrehman107 · 2023-10-12T18:29:28Z

Oh, I'll look into this. I thought I'd maybe base my test cases solely on the return statements.
Can you maybe suggest an example for the other test cases possible? I can maybe take direction from that

abdurrehman107 · 2023-10-13T05:39:37Z

@janiskemper whats confusing me is that if I take the if statement
if hetznerCluster.Spec.ControlPlaneEndpoint == nil and add it to the test case, what do I put in the want? Do I just put in true (I think if the function is at this stage of the program, then it will eventually return true)? Or am I mistaking?

…ge to hetznercluster_controller.go

abdurrehman107 · 2023-10-13T14:47:31Z

@janiskemper Can you please review the latest commit?

janiskemper · 2023-10-13T18:11:46Z

the main part of the test should also be to check the changes that have been made to the hetznercluster... that you don't test currently"

abdurrehman107 · 2023-10-14T06:05:32Z

Ahan. @janiskemper can you please elaborate on that a little. Like are we talking about the changes to hetznerCluster.Spec.ControlPlaneEndpoint.Host and hetznerCluster.Spec.ControlPlaneEndpoint.Port?

janiskemper · 2023-10-16T06:53:36Z

about hetznerCluster.Spec.ControlPlaneEndpoint in general, yeys

abdurrehman107 · 2023-10-16T07:21:23Z

I'm sorry @janiskemper I feel a little lost as to what am I missing. I'll add another commit maybe rephrasing the test case names. I've tried addressing changes being made to the ControlPlaneEndpoint.Host and ControlPlaneEndpoint.Port in the first two test cases. Can you please go through it?
I'd love feedback at what I'm missing

janiskemper · 2023-10-16T07:58:46Z

you just have to also include test cases for changes of hetznerCluster.Spec.ControlPlaneEndpoint.. At the moment you only test the return boolean in https://github.com/syself/cluster-api-provider-hetzner/pull/1004/files#diff-17dd1bcc2376d005c272189be910b2625e9a01d4bc08d71663eb972ed88f0df6R877.
Just not focus on coding for now.. Look at the function from a high level perspective and think about the logic it contains and for stuff that you might want to test for

abdurrehman107 · 2023-10-16T14:53:16Z

Hi @janiskemper. Upon going through the recent activity, I think I maybe have a better idea of what you're asking. Please verify if my understanding is right about this.
So the major change that is taking palce in the function is the changing of state in ControlPlaneEndpoint. So do you want me to pass individual values in test case and then see how ControlPlaneEndpoint.Host and ControlPlaneEndpoint.Port responds to that?
Maybe something like:
For instance if I pass hetznerCluster.Spec.ControlPlaneEndpoint.Host == "" and hetznerCluster.Spec.ControlPlaneEndpoint.Port == 0 then I should make sure that I final value set for the Host and Port should be hetznerCluster.Status.ControlPlaneLoadBalancer.IPv4 and int32(hetznerCluster.Spec.ControlPlaneLoadBalancer.Port).
Am I reading this right?

janiskemper · 2023-10-16T15:12:31Z

yes, exactly!

abdurrehman107 · 2023-10-16T18:14:25Z

@janiskemper. So I was earlier adding test cases, but I think the test cases are fine, like I do not need to add more. The condition I added should be okay right? Or would you recommend adding another test case?
If everything is okay, then do I just squash the commits and push?

janiskemper · 2023-10-17T07:03:03Z

can you maybe take a step back and think about every test that you could potentially do? Because I think this structure of the fixed table-driven test holds you back. Just make a list of tests that could be implemented. I believe we are still missing out many

abdurrehman107 · 2023-10-17T13:17:31Z

I can test if ControlPlaneLoadbalancer.IPV4 is "<nil>" then the variables defaultHost and defaultPort should not be nil. (I dont think I can do this though, because these are local variables and they are not being passed in the function. Or can I?)
I can also try another test which checks if Host is empty like hetznerCluster.Spec.ControlPlaneEndpoint.Host == "" then as the test ends it should be equal to hetznerCluster.Status.ControlPlaneLoadBalancer.IPv4.
I can do the same thing with hetznerCluster.Spec.ControlPlaneEndpoint.Port == 0 and check if it equals hetznerCluster.Spec.ControlPlaneLoadBalancer.Port

Am I approaching this correct?

janiskemper · 2023-10-17T13:57:53Z

just for an example: What if controlPlaneLoadBalancer is not enabled? Then other things happen.. You can really go through the whole logic and pretty much any possible combination of things that might happen can be (theoretically) tested.

abdurrehman107 · 2023-10-17T16:20:17Z

Okay just clearing out with this example, wouldn't the answer just be that the want variable will just turn to false since my function just returns false if controlPlaneLoadBalancer is not enabled. I'm sorry @janiskemper. Can you maybe try with another example?
Oh also one more question. I'm currently only suggesting things looking at maybe how my function is moving. Do you want me to maybe move out of the function? The only variable that my function changes with its return value is hetznerCluster.Status.Ready.

func SetControlPlaneEndpoint(hetznerCluster *infrav1.HetznerCluster) (bool) {
	if hetznerCluster.Spec.ControlPlaneLoadBalancer.Enabled {
		if hetznerCluster.Status.ControlPlaneLoadBalancer.IPv4 != "<nil>" {
			defaultHost := hetznerCluster.Status.ControlPlaneLoadBalancer.IPv4
			defaultPort := int32(hetznerCluster.Spec.ControlPlaneLoadBalancer.Port)

			if hetznerCluster.Spec.ControlPlaneEndpoint == nil {
				hetznerCluster.Spec.ControlPlaneEndpoint = &clusterv1.APIEndpoint{
					Host: defaultHost,
					Port: defaultPort,
				}
				return true
			} else {
				if hetznerCluster.Spec.ControlPlaneEndpoint.Host == "" {
					hetznerCluster.Spec.ControlPlaneEndpoint.Host = defaultHost
				}
				if hetznerCluster.Spec.ControlPlaneEndpoint.Port == 0 {
					hetznerCluster.Spec.ControlPlaneEndpoint.Port = defaultPort
				}
				return true
			}
		}
	} else if hetznerCluster.Spec.ControlPlaneEndpoint != nil {
		return true
	}
	return false
}

Can you go through the function maybe and point another example over here. I'd really appreciate it

janiskemper · 2023-10-18T08:19:32Z

you can check what happens if load balancer is not enabled and if the controlPlaneEndpoint is nil / not nil.
You can check what happens if the load balancer is enabledand the IPv4 is "", or is not.
You can check what happens if ControlPlaneEndpoint is nil or not nil and what happens if host and port are set or not.
You can check what happens for different values of ControlPlaneEndpoint and ControlPlaneLoadBalancer.
You can check all of that!

abdurrehman107 · 2023-10-18T11:26:57Z

🤦 . I'm sorry @janiskemper. Earlier you mentioned about checking changes made to hetznercluster, I think I got very zoomed in on that only. I was trying to avoid want statements, which is why it did not make sense to me. Can you please go through the latest commit. I feel that there can be more test cases with more values, but they'll more or less be same. Is there still something that I'm missing.

janiskemper · 2023-10-18T12:41:42Z

maybe you can do the following: I think the table-driven test really makes you think very one-directional. Can you completely get rid of it and do one test case after another? I think that would really help you... I mentioned a lot of tests in my previous comment that you can potentially do.

abdurrehman107 · 2023-10-18T12:59:53Z

I thought I made test cases for all the test cases you suggested? I'm defo missing some major point. I can discard all my tests and restart from line one. Would you suggest I do that?

janiskemper · 2023-10-18T13:31:42Z

you implented table driven tests. Can you re-implement what you currently have (and maybe new tests) in a manner that doesn't use table-driven tests?

abdurrehman107 · 2023-10-18T13:48:34Z

Writing a function for each test counts? Something similar to whats mentioned in this link [https://golang.cafe/blog/golang-table-test-example]

janiskemper · 2023-10-18T14:44:14Z

yes, but maybe a pattern that uses t.Run("InvalidMajorVersion", func(t *testing.T) {// test code} within a test is better.

abdurrehman107 · 2023-10-18T16:38:08Z

Aight. I'll get started with this right away.

janiskemper · 2023-10-24T11:08:29Z

closing as this is replaced by #1023

added function to update control plane endpoint

1d940ce

syself-bot bot added size/M Denotes a PR that changes 50-200 lines, ignoring generated files. area/code Changes made in the code directory labels Oct 10, 2023

abdurrehman107 commented Oct 10, 2023

View reviewed changes

controllers/hetznercluster_controller.go Show resolved Hide resolved

batistein marked this pull request as draft October 10, 2023 16:38

Added test for SetControlPlaneEndpoint() function

7c71490

Added unit test in hetznercluster_controller_test.go, made minor chan…

5858e83

…ge to hetznercluster_controller.go

Rephrased names of test

1c49b5c

Added condition to check Host and Port in unit test

cd1749d

janiskemper closed this Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱Added function to update control plane endpoint #1004

🌱Added function to update control plane endpoint #1004

abdurrehman107 commented Oct 10, 2023

abdurrehman107 commented Oct 12, 2023

janiskemper commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023

janiskemper commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023 •

edited

Loading

abdurrehman107 commented Oct 12, 2023

janiskemper commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023

janiskemper commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023

abdurrehman107 commented Oct 13, 2023

abdurrehman107 commented Oct 13, 2023

janiskemper commented Oct 13, 2023

abdurrehman107 commented Oct 14, 2023

janiskemper commented Oct 16, 2023

abdurrehman107 commented Oct 16, 2023

janiskemper commented Oct 16, 2023

abdurrehman107 commented Oct 16, 2023 •

edited

Loading

janiskemper commented Oct 16, 2023

abdurrehman107 commented Oct 16, 2023

janiskemper commented Oct 17, 2023

abdurrehman107 commented Oct 17, 2023 •

edited

Loading

janiskemper commented Oct 17, 2023

abdurrehman107 commented Oct 17, 2023

janiskemper commented Oct 18, 2023

abdurrehman107 commented Oct 18, 2023

janiskemper commented Oct 18, 2023

abdurrehman107 commented Oct 18, 2023

janiskemper commented Oct 18, 2023

abdurrehman107 commented Oct 18, 2023

janiskemper commented Oct 18, 2023

abdurrehman107 commented Oct 18, 2023

janiskemper commented Oct 24, 2023

🌱Added function to update control plane endpoint #1004

🌱Added function to update control plane endpoint #1004

Conversation

abdurrehman107 commented Oct 10, 2023

abdurrehman107 commented Oct 12, 2023

janiskemper commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023

janiskemper commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023 • edited Loading

abdurrehman107 commented Oct 12, 2023

janiskemper commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023

janiskemper commented Oct 12, 2023

abdurrehman107 commented Oct 12, 2023

abdurrehman107 commented Oct 13, 2023

abdurrehman107 commented Oct 13, 2023

janiskemper commented Oct 13, 2023

abdurrehman107 commented Oct 14, 2023

janiskemper commented Oct 16, 2023

abdurrehman107 commented Oct 16, 2023

janiskemper commented Oct 16, 2023

abdurrehman107 commented Oct 16, 2023 • edited Loading

janiskemper commented Oct 16, 2023

abdurrehman107 commented Oct 16, 2023

janiskemper commented Oct 17, 2023

abdurrehman107 commented Oct 17, 2023 • edited Loading

janiskemper commented Oct 17, 2023

abdurrehman107 commented Oct 17, 2023

janiskemper commented Oct 18, 2023

abdurrehman107 commented Oct 18, 2023

janiskemper commented Oct 18, 2023

abdurrehman107 commented Oct 18, 2023

janiskemper commented Oct 18, 2023

abdurrehman107 commented Oct 18, 2023

janiskemper commented Oct 18, 2023

abdurrehman107 commented Oct 18, 2023

janiskemper commented Oct 24, 2023

abdurrehman107 commented Oct 12, 2023 •

edited

Loading

abdurrehman107 commented Oct 16, 2023 •

edited

Loading

abdurrehman107 commented Oct 17, 2023 •

edited

Loading