🌱 Update api dependency and remove unnecessary featureGate enabling part
#687
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RokibulHasan7 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 |
RokibulHasan7
changed the title
api dependency and remove unnecessary featureGate enabling part api dependency and remove unnecessary featureGate enabling part
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #687 +/- ##
==========================================
- Coverage 63.64% 63.37% -0.28%
==========================================
Files 182 185 +3
Lines 17634 17771 +137
==========================================
+ Hits 11223 11262 +39
- Misses 5480 5578 +98
Partials 931 931
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
RokibulHasan7
force-pushed
the
rmv-fg
branch
4 times, most recently
from
b29c2e9
to
2ef0e77
Compare
pkg/operator/helpers/helpers.go
Outdated
| @@ -775,11 +767,23 @@ func ConvertToFeatureGateFlags(component string, features []operatorapiv1.Featur | |||
| continue | |||
| } | |||
|
|
|||
| if feature.Mode == operatorapiv1.FeatureGateModeTypeDisable && defaultFeature.Default { | |||
| if feature.Mode == operatorapiv1.FeatureGateModeTypeEnable { | |||
pkg/operator/helpers/helpers_test.go
Outdated
| @@ -1572,15 +1573,15 @@ func TestConvertToFeatureGateFlags(t *testing.T) { | |||
| { | |||
| name: "unset", | |||
| features: []operatorapiv1.FeatureGate{}, | |||
| desiredFlags: []string{}, | |||
| desiredFlags: []string{"--feature-gates=ClusterClaim=true", "--feature-gates=AddonManagement=true"}, | |||
There was a problem hiding this comment.
if it is unset, should we keep the desireFlags unset?
There was a problem hiding this comment.
If the user's provided featureGate is nil, the default features will be enabled automatically. So, the desiredFlags set to the default featureGate
There was a problem hiding this comment.
I am thinking of the case: if we set the default value in the flags explicitly(TestFeature=false), once we change the default value in the future(TestFeature=true), will the feature be changed(false to true)? This is going to be hard to decide because we do not know whether the old value(TestFeature=false) is set by default or users. What is our expectation in this case?
There was a problem hiding this comment.
If the user explicitly sets a value for a feature gate, we’re always taking that as the defined value. If the user doesn’t set any value for feature gates where Default=True, then we’re enabling those feature gates.
There was a problem hiding this comment.
If the user doesn’t set any value for feature gates(TestFeature) where Default=False, so this PR will set the flag TestFeature=False right? If we change the TestFeature Default=True in the next release, when upgrading from this release to the next release, how do we know if the old flag TestFeature=False is set by users on purpose or set by us by default? I am asking because I think if the user intentionally disabled a feature by setting TestFeature=False, we should not enable it when upgrading.
There was a problem hiding this comment.
Got it, updated. PTAL @zhujian7
|
/cc @qiujian16 |
RokibulHasan7
force-pushed
the
rmv-fg
branch
5 times, most recently
from
a921e79
to
78a76dc
Compare
Signed-off-by: Rokibul Hasan <[email protected]>
RokibulHasan7
force-pushed
the
rmv-fg
branch
from
78a76dc
to
149ed52
Compare
Summary
Related issue(s)
Fixes #