Don't override default values when applying partial features.yaml configmap #7379
Conversation
Signed-off-by: Pierangelo Di Pilato <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pierDipi 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 |
knative-prow
bot
added
approved
knative-prow
bot
requested review from
creydr,
odacremolbap,
Cali0707 and
Leo6Leo
|
/cherry-pick release-1.11 |
|
@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.11 in a new PR and assign it to you. In response to this:
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/test-infra repository. |
|
/cherry-pick release-1.10 |
|
@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you. In response to this:
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/test-infra repository. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7379 +/- ##
==========================================
- Coverage 76.87% 76.72% -0.15%
==========================================
Files 252 253 +1
Lines 13822 13903 +81
==========================================
+ Hits 10625 10667 +42
- Misses 2667 2702 +35
- Partials 530 534 +4
☔ View full report in Codecov by Sentry. |
| require.Nil(t, err) | ||
| require.NotNil(t, f) | ||
|
|
||
| if !f.IsDisabled(KReferenceGroup) && !f.IsEnabled(KReferenceGroup) { |
There was a problem hiding this comment.
@pierDipi , I've reviewed the part where we call NewFlagsConfigFromMap with an empty string map. If I understand correctly, this will return an empty flag. Wouldn't this potentially cause an issue at line 65? (nvm, it only checks for nil. The flag is empty, and it doesn't mean it is nil)
Also, I noted that with use of KReferenceGroup as a feature flag name. Is this specifically for testing purposes? Why we are using KReferenceGroup? And the if statement here is used to validate that KReferenceGroup only allow values from "Disabled" or "Enabled" right? How can that be related to "TestShouldNotOverrideDefaults"?
Thanks for helping me clarify this!
There was a problem hiding this comment.
I've reviewed the part where we call NewFlagsConfigFromMap with an empty string map. If I understand correctly, this will return an empty flag. Wouldn't this potentially cause an issue at line 65? (nvm, it only checks for nil. The flag is empty, and it doesn't mean it is nil)
Exactly
Also, I noted that with use of KReferenceGroup as a feature flag name. Is this specifically for testing purposes? Why we are using KReferenceGroup? And the if statement here is used to validate that KReferenceGroup only allow values from "Disabled" or "Enabled" right? How can that be related to "TestShouldNotOverrideDefaults"?
This assertion is the "regression test" for the original issue and if you remove the solution this assertion will fail, there is no specific reason for using that feature flag KReferenceGroup, another one would have worked as well
Signed-off-by: Pierangelo Di Pilato <[email protected]>
|
/test upgrade-tests |
|
@pierDipi: new pull request created: #7385 In response to this:
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/test-infra repository. |
|
@pierDipi: new pull request created: #7386 In response to this:
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/test-infra repository. |
Fixes #7254
Proposed Changes
Pre-review Checklist
Release Note
Docs