Fix deletion protection unit test #106376
Conversation
The test should not depend on current set of default feature gates, it should always ensure the ones necessary for the tests are set.
k8s-ci-robot
added
release-note-none
|
@jsafrane: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
k8s-ci-robot
added
do-not-merge/needs-sig
|
/sig storage |
k8s-ci-robot
added
the
sig/storage
| @@ -678,6 +678,9 @@ func TestAnnealMigrationAnnotations(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestUpdateFinalizer(t *testing.T) { | |||
| // This set of tests ensures that protection finalizer is removed when CSI migration is disabled | |||
| // and PV controller needs to remove finalizers added by the external-provisioner. | |||
| defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIMigrationGCE, false)() | |||
There was a problem hiding this comment.
Should this also toggle CSIMigration feature gate?
There was a problem hiding this comment.
This change does pass the test for #104722 but I don't quite understand why only one test case enables CSIMigrationGCE? and for the CSIMigration flag, it is enabled now by default in k/k so I think migratedDriverGates: []featuregate.Feature{features.CSIMigration} will not do anything
There was a problem hiding this comment.
This test case is only using gce pd as an example of a migrated driver. It's not testing every single migrated driver we support.
Even though CSIMigration feature gate is on by default, the unit test can exercise turning the feature gate off, hence why we should still explicitly toggle feature gates in the unit tests.
There was a problem hiding this comment.
Yeah what I mean is perhaps the test should explicitly turn off CSIMigration gate instead of "not turn on" such gate
|
CC @deepakkinni |
|
/retest |
|
are we able to merge this? or I can put this change into #104722 as well |
|
/approve |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepakkinni, jsafrane, xing-yang 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 |
Unit tests should not depend on current set of default feature gates, they should always ensure the ones necessary for the tests are set.
Helps with #104722 after #105773
/kind bug
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: