feat: adds EnabledWhen to the feature builder #1057
Conversation
|
so this EnabledWhen can be used to replace PreCondition in some way? |
|
|
added hold label to get additional review from @bartoszmajsak before allowing auto-merge |
| @@ -58,7 +59,7 @@ var _ = Describe("feature cleanup", func() { | |||
| Expect(featuresHandler.Apply()).Should(Succeed()) | |||
|
|
|||
| // then | |||
| Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName)). | |||
| Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace)). | |||
There was a problem hiding this comment.
Any reason to remove secret name as a param? With this change we hardcode the secret name in the func itself, not sure we want that.
There was a problem hiding this comment.
This was removed as linter complained as we were passing in the same secret name each time IIRC
| Expect(featuresHandler.Apply()).Should(Succeed()) | ||
|
|
||
| // then | ||
| Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace)). |
There was a problem hiding this comment.
I wonder if we shouldn't test something different here. We should ensure nothing that is supposed to be created by the defined feature (including tracker) should exist in the cluster.
On top of that, Consistently might be better instead of Eventually (which can pass in the first run). The clean-up from the previous run might not be done yet on the cluster when this test is executed. I see you are handling it with the current implementation by checking if ns is in a deletion state, though I believe switching it to Consistently should make it safer if we happen to rework the test fixture, but keep the expectations the same.
| }) | ||
|
|
||
| func createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName string) func() error { | ||
| func createdSecretHasOwnerReferenceToOwningFeature(namespace string) func() error { |
There was a problem hiding this comment.
In createdSecretHasOwnerReferenceToOwningFeature shouldn't we ensure we look up the FT matching the feature, not assuming there's a FT matching any Feature in the cluster that was related to "test-secret"? In other words I would think this function also expects a Feature to be passed by only reading the signature. Otherwise we assume a bit too much internally.
Co-authored-by: Bartosz Majsak <[email protected]>
Co-authored-by: Bartosz Majsak <[email protected]>
Co-authored-by: Bartosz Majsak <[email protected]>
cam-garrison
force-pushed
the
enabled-when
branch
from
b683af0
to
6c15231
Compare
|
linter fails when nolint has preceding space and fails when it does not have preceding space : / fixing this tmrw |
Co-authored-by: Bartosz Majsak <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bartoszmajsak, zdtsw 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 |
openshift-merge-bot
bot
merged commit bff2e6a
into
opendatahub-io:incubation
* rebase on top of latest inc * fix linter * rename test func * Update pkg/feature/builder.go Co-authored-by: Bartosz Majsak <[email protected]> * Update pkg/feature/builder.go Co-authored-by: Bartosz Majsak <[email protected]> * fix err var name * Update tests/integration/features/cleanup_int_test.go Co-authored-by: Bartosz Majsak <[email protected]> * fix linter, add ctx * re-remove pesky whitespace * move logic to apply and improve tests * ensure feature tracker is also removed * Update tests/integration/features/cleanup_int_test.go Co-authored-by: Bartosz Majsak <[email protected]> --------- Co-authored-by: Bartosz Majsak <[email protected]> (cherry picked from commit bff2e6a)
* rebase on top of latest inc * fix linter * rename test func * Update pkg/feature/builder.go Co-authored-by: Bartosz Majsak <[email protected]> * Update pkg/feature/builder.go Co-authored-by: Bartosz Majsak <[email protected]> * fix err var name * Update tests/integration/features/cleanup_int_test.go Co-authored-by: Bartosz Majsak <[email protected]> * fix linter, add ctx * re-remove pesky whitespace * move logic to apply and improve tests * ensure feature tracker is also removed * Update tests/integration/features/cleanup_int_test.go Co-authored-by: Bartosz Majsak <[email protected]> --------- Co-authored-by: Bartosz Majsak <[email protected]> (cherry picked from commit bff2e6a)
Description
This PR introduces a featureBuilder function that enables conditional toggling of features named
EnabledWhen. Features can now be activated or deactivated based on specific conditions such as the target platform, management mode, or the presence of particular resources in the cluster.Building on the previous PR linked below which we didn't finish up, this update ensures that feature.Cleanup() is called if a feature's EnabledWhen() status changes from true to false across multiple reconciles. Additionally, this PR includes enhanced testing to validate these changes.
JIRA issue:
https://issues.redhat.com/browse/RHOAIENG-8540
re-doing and finishing up work from PR #793
How Has This Been Tested?
Wrote additional cleanup integration tests - using an example function checking if a certain namespace exists to enable the feature.
Ensured the feature applied when the namespace exists and that the feature was cleaned up on "reconcile" when the namespace no longer exists.
Screenshot or short clip:
Merge criteria: