Fix KubeLB option precedence enforced over enabled#7999
Fix KubeLB option precedence enforced over enabled#7999KhizerRehan wants to merge 1 commit intokubermatic:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@shinushaju kindly, check behaviour matrix. It should behave according to precedence order defined. However the issue of not returning References: |
|
/retest |
|
LGTM label has been added. DetailsGit tree hash: 6f66d3c5443c77546bc06e7bee3bcdc83c0c7bc9 |
|
/hold Concern/Clarification Added Priority order: As discussed with @ahmadhamzh, it was mentioned that
If that is the expected behavior, and
This raises another question: if we want to enable KubeLB for all In my opinion, Otherwise, the only way to hide KubeLB would be to set both flags to Based on this, I’m trying to understand whether the logic introduced in the PR aligns with this intended behavior. Therefore this PR is on hold for further discussion |
ahmadhamzh
left a comment
There was a problem hiding this comment.
so from what i understand if seedSettings?.kubelb?.enableForAllDatacenters is true then kubeLB will be available in all datacenters regardless of the dc.spec.kubelb?.enabled value,
@ahmedwaleedmalik is that correct ?
| if (datacenter.spec.kubelb?.enforced) { | ||
| return true; | ||
| } | ||
| if (datacenter.spec.kubelb?.enabled === false) { |
There was a problem hiding this comment.
according to my understand this sholdn't return false unless the seedSettings?.kubelb?.enableForAllDatacenters is also false, so we need to check that before returning false
What this PR does / why we need it:
This PR fixes KubeLB option visibility so that enforced takes precedence over the enabled flag
Behaviour Matrix
Which issue(s) this PR fixes:
Fixes #7902
What type of PR is this?
/kind bug
Special notes for your reviewer:
Does this PR introduce a user-facing change? Then add your Release Note here:
Documentation:
Test issue: