Avoid hardcoding port in clusterclient.go #734
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
|
I would also recommend to rename |
| cluster, err := c.GetClusterObject(clusterName, namespace) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // clusterEndpoint can be passed as hostname or hostname:port, if port is not present | ||
| // the default port 443 will be applied. |
There was a problem hiding this comment.
Should we have this and a better description in an exported comment?
| @@ -406,15 +408,32 @@ func newDeleteOptions() *metav1.DeleteOptions { | |||
| } | |||
|
|
|||
| // TODO: Test this function | |||
| func (c *client) UpdateClusterObjectEndpoint(controlPlaneIP, clusterName, namespace string) error { | |||
| func (c *client) UpdateClusterObjectEndpoint(clusterEndpoint, clusterName, namespace string) error { | |||
There was a problem hiding this comment.
Do we know what's using this method currently?
There was a problem hiding this comment.
clusterdeployer.go in updateClusterEndpoint
In clusterclient.go client.UpdateClusterObjectEndpoint checks if the value fetched from the provider is in the host:port format, if no port is present appends the default API server port (443). Fixes: #559
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| // TODO: Test this function | ||
| func (c *client) UpdateClusterObjectEndpoint(controlPlaneIP, clusterName, namespace string) error { | ||
| func (c *client) UpdateClusterObjectEndpoint(clusterEndpoint, clusterName, namespace string) error { |
There was a problem hiding this comment.
Should we add a simple test and remove the TODO as part of this change?
Generally I would agree with this, however the Deployer interface in general is a workaround and only required for building custom clusterctl binaries for each provider. Rather than iterate on this interface we should instead focus on replacing the Deployer interface and properly extending |
|
/lgtm |
There was a problem hiding this comment.
/approve
@frapposelli We can follow up with some unit tests in a new PR.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frapposelli, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@vincepri sounds good 👍 |
What this PR does / why we need it:
In clusterclient.go client.UpdateClusterObjectEndpoint checks if the value fetched from the provider is in the host:port format, if no port is present appends the default API server port (443).
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 #559
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: