Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🌱 Add SetControlPlaneEndpoint function #1023

Merged
merged 1 commit into from
Nov 3, 2023
Merged

🌱 Add SetControlPlaneEndpoint function #1023

merged 1 commit into from
Nov 3, 2023

Conversation

abdurrehman107
Copy link
Contributor

@abdurrehman107 abdurrehman107 commented Oct 23, 2023 •
edited
Loading

What this PR does / why we need it:
The following PR addresses the issue #977, and aims to add the function SetControlPlaneEndpoint() and unit tests for it.
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:

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

@janiskemper janiskemper mentioned this pull request Oct 24, 2023
Closed
3 tasks
janiskemper
janiskemper requested changes Oct 24, 2023
View reviewed changes
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
@abdurrehman107
Copy link
Contributor Author

@janiskemper. I've made the required changes. Can you please review again? I've squashed my commits

janiskemper
janiskemper requested changes Oct 25, 2023
View reviewed changes
controllers/hetznercluster_controller.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
@abdurrehman107
Copy link
Contributor Author

@janiskemper . I've implemented the recommended changes. Can you please take a look?

janiskemper
janiskemper requested changes Oct 26, 2023
View reviewed changes
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
janiskemper
janiskemper requested changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@janiskemper janiskemper left a comment

Choose a reason for hiding this comment

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

to complete the tests, you should have also two more tests that are rather similar to the last one you have, where you already have a host or a port set so that it does not get updates.

Also you should have a test where both is set already and also doesn't get replaced (maybe by a different value from ControlPlaneLoadBalancer.

So: you can implement three more tests!

controllers/hetznercluster_controller.go Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller_test.go Outdated Show resolved Hide resolved
controllers/hetznercluster_controller.go Outdated Show resolved Hide resolved
@abdurrehman107
Copy link
Contributor Author

@janiskemper I've made the changes. I request you take another look

@janiskemper
Copy link
Contributor

I believe that you haven't implemented the additional tests right?

@abdurrehman107
Copy link
Contributor Author

@janiskemper I did implement the three additional tests you recommended

@janiskemper
Copy link
Contributor

true. My bad. You can resolve the two unresolved conversations and then you are done I guess!

@abdurrehman107
Copy link
Contributor Author

@janiskemper I think we should be good now. I had added a port value for ControlPlaneLoadBalancer.Port too. I think that was unnecessary. I removed that, hence the two commits.
I've ammended that. Please take a look.

@janiskemper janiskemper marked this pull request as draft November 2, 2023 13:53
@janiskemper
Copy link
Contributor

you can squash now

@abdurrehman107
Copy link
Contributor Author

@janiskemper i've squashed the commits. Learnt so much with this issue. Thank you

@janiskemper
Copy link
Contributor

did you call make verify and make test? If so, we can mark it as "ready for review"

@janiskemper janiskemper marked this pull request as ready for review November 3, 2023 10:13
@syself-bot syself-bot bot added the area/code Changes made in the code directory label Nov 3, 2023
@janiskemper
Copy link
Contributor

There is still an issue with the CI and hcloud private networks. Merging anyway

@janiskemper
Copy link
Contributor

thanks @razashahid107 for this PR :)

@janiskemper janiskemper changed the title Added SetControlPlaneEndpoint() function and unit tests for the funciotn 🌱 Add SetControlPlaneEndpoint function Nov 3, 2023
@janiskemper janiskemper merged commit 1f67c6f into syself:main Nov 3, 2023
11 of 12 checks passed
@abdurrehman107
Copy link
Contributor Author

I loved doing this one. Got to learn so much. Thank you for being supportive @janiskemper @batistein

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janiskemper janiskemper janiskemper approved these changes

Assignees
No one assigned
Labels
area/code Changes made in the code directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add utility function to update controlPlaneEndpoint
2 participants
@abdurrehman107 @janiskemper