Fix priority between network and DHCP server #2269
Conversation
k8s-ci-robot
added
the
area/provider/ibmcloud
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dharaneeshvrd The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/cc @Karthik-K-N |
There was a problem hiding this comment.
https://github.com/Karthik-K-N/cluster-api-provider-ibmcloud/blob/c4171f21acc320fdfe17b812c98be908653134f7/cloud/scope/powervs_cluster.go#L870, This comment needs an update as now we are not allowing to set both network and DHCP details in spec.
| }, | ||
| DHCPServer: &DHCPServer{ | ||
| Name: ptr.To("capi-dhcp"), | ||
| }, |
There was a problem hiding this comment.
May lets also add test with network name - dhcp id , network id -dhcp id combination?
dharaneeshvrd
force-pushed
the
fix-nw-priority
branch
from
f76ccab to
09862db
Compare
k8s-ci-robot
added
size/L
|
Addressed the comments, ptal @Karthik-K-N |
There was a problem hiding this comment.
Do you think is there a way to handle/inform about this scenario, Correct me if my understanding is wrong.
What if user sets network.Name="abc" the network "abc" does not exist in cloud. I think our controller will create a DHCP network DHCPSERVERabc_Private. Now user has set "abc" as network in spec but actually its DHCPSERVERabc_Private
dharaneeshvrd
force-pushed
the
fix-nw-priority
branch
from
09862db to
7cfc5ac
Compare
|
@Karthik-K-N |
|
True, Thats the only thing we can do, along with that should we update spec.Network.Name with the newly created DHCP network name? will that be allowed? |
Can you please elaborate this? Do you mean to update the spec once we created the DHCP network? Just wondering since we won't follow that generally right! |
True we wont follow it, Just thinking. |
k8s-ci-robot
added
the
needs-rebase
|
Or else how about strict validation like ID, when network.Name is set it should exist or else return error? Most of the time we ommit network field and controller will autocreate the resource so it wont be a breaking change. |
Do not allow to set both network and DHCP server details which will create conflict while creating new DHCP server on whether to use network name or DHCP server name
dharaneeshvrd
force-pushed
the
fix-nw-priority
branch
from
7cfc5ac to
f46c35c
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Yeah that might be an option if we consider that field only for non DHCP networks. Anyway we are validating with that intention only. But only concern is that it would deviate from the other places on how we treat the name field of a resource. |
True that, Though the use case is very minimal, I am not so sure about showing wrong info about network name in spec. May be lets discuss this in the community meeting and we can either go with that or keep as it is and add note like now. |
|
@dharaneeshvrd: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What this PR does / why we need it:
Fix priority between network and DHCP server
Do not allow to set both network and DHCP server details which will create conflict while creating new DHCP server on whether to use network name or DHCP server name
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 #2159
Special notes for your reviewer:
/area provider/ibmcloud
Release note: