Dependent features #274

Tymek · 2023-10-05T12:33:25Z

Description

Introducing concept of "dependency" on a feature toggle. Feature toggle, in order to be enabled an return a variant, can require the specified state of another feature toggle. There will be a dependencies property on each feature.

{
 ...
 "dependencies": {
   "feature": "string-featureId",
   "enabled: true, // boolean, state that is expected defaults to `true`
   "variants": ["variant1", "variant2"] // optional, variants allowed for feature to be enabled
 }
}

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Unit tests
Spec Tests
Integration tests / Manual Tests

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

for more information, see https://pre-commit.ci

coveralls · 2023-10-05T22:50:19Z

coverage: 96.885% (+0.007%) from 96.878% when pulling aa55c07 on feat/feature-dependencies into b1d5b70 on main.

tests/unit_tests/api/test_feature.py

for more information, see https://pre-commit.ci

kwasniew

Looks good to me however I'm not a Python dev so would appreciate some Pythonista eyes on it :) I'd also add a test showing that we don't record a metrics for the parent feature.

kwasniew · 2023-10-09T08:59:33Z

UnleashClient/__init__.py

+        dependency_feature = self.features[dependency_name]
+
+        if not dependency_feature:
+            LOGGER.warning("Feature dependency not found. %s", dependency_name)


in Node we have a mechanism to only log this message once. I'm assuming we don't do the same thing in Python SDK

Not a thing in the Python SDK at the moment, unfortunately. Not against adding it, but that should probably be a different PR

UnleashClient/features/Feature.py

tests/unit_tests/test_client.py

sighphyre

Overall LGTM, I've left some thoughts, which are optional but should be considered, so I'm going to mark this as changes requested so this doesn't get accidentally merged

sighphyre · 2023-10-12T13:09:08Z

Looks good to me however I'm not a Python dev so would appreciate some Pythonista eyes on it :) I'd also add a test showing that we don't record a metrics for the parent feature.

Agree with @kwasniew on the test, please!

Tymek · 2023-10-24T07:58:16Z

UnleashClient/features/Feature.py

@@ -75,20 +82,20 @@ def _count_variant(self, variant_name: str) -> None:
        """
        self.variant_counts[variant_name] = self.variant_counts.get(variant_name, 0) + 1

-    def is_enabled(self, context: dict = None) -> bool:
+    def is_enabled(self, context: dict = None, skip_stats: bool = False) -> bool:


Is there a more elegant way to do it? Without passing skip_stats around.

Can't think of a way without rearchitecting this a little. It should be possible to move the count metrics methods to public ones and only call those in the is_enabled in the UnleashClient but... I'm not sure that's worth it. This is all internal at the moment and when Yggdrasil arrives here this will go away anyway.

I'm okay with this as it stands. I think we can do better but I also don't think it's worth it to do so. I think this is a reasonable tradeoff

sighphyre

Thumbs up

Tymek and others added 7 commits October 5, 2023 14:30

update specs

9939341

add dependencies to feature

d7762d0

[pre-commit.ci] auto fixes from pre-commit.com hooks

62cea8b

for more information, see https://pre-commit.ci

delay failing test issue

3e05b42

[pre-commit.ci] auto fixes from pre-commit.com hooks

3860408

for more information, see https://pre-commit.ci

feat: feature dependencies

da5ee19

[pre-commit.ci] auto fixes from pre-commit.com hooks

99ffa8d

for more information, see https://pre-commit.ci

Tymek commented Oct 6, 2023

View reviewed changes

tests/unit_tests/api/test_feature.py Outdated Show resolved Hide resolved

Tymek and others added 4 commits October 6, 2023 13:19

test feature dependencies

6722c20

[pre-commit.ci] auto fixes from pre-commit.com hooks

c243d90

for more information, see https://pre-commit.ci

add more dependency test cases

d408e12

[pre-commit.ci] auto fixes from pre-commit.com hooks

017e54f

for more information, see https://pre-commit.ci

Tymek requested review from ivanklee86, sighphyre and andreas-unleash October 6, 2023 12:21

Tymek marked this pull request as ready for review October 6, 2023 12:21

kwasniew reviewed Oct 9, 2023

View reviewed changes

sighphyre reviewed Oct 12, 2023

View reviewed changes

UnleashClient/features/Feature.py Outdated Show resolved Hide resolved

sighphyre reviewed Oct 12, 2023

View reviewed changes

UnleashClient/features/Feature.py Outdated Show resolved Hide resolved

sighphyre reviewed Oct 12, 2023

View reviewed changes

tests/unit_tests/test_client.py Outdated Show resolved Hide resolved

sighphyre requested changes Oct 12, 2023

View reviewed changes

Tymek added 4 commits October 23, 2023 17:18

Merge branch 'main' into feat/feature-dependencies

6e19900

refactor: dependency test from bootstrapping

4c647e5

fix: feature dependencies arguments

ec4fea8

fix: don't send metrics for parent features

aa55c07

Tymek commented Oct 24, 2023

View reviewed changes

Tymek requested a review from sighphyre October 24, 2023 09:28

sighphyre approved these changes Oct 24, 2023

View reviewed changes

Tymek merged commit 5133699 into main Oct 24, 2023
34 checks passed

Tymek deleted the feat/feature-dependencies branch October 24, 2023 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependent features #274

Dependent features #274

Tymek commented Oct 5, 2023 •

edited

Loading

coveralls commented Oct 5, 2023 •

edited

Loading

kwasniew left a comment

kwasniew Oct 9, 2023

sighphyre Oct 12, 2023

sighphyre left a comment

sighphyre commented Oct 12, 2023

Tymek Oct 24, 2023

sighphyre Oct 24, 2023

sighphyre left a comment

Dependent features #274

Dependent features #274

Conversation

Tymek commented Oct 5, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

coveralls commented Oct 5, 2023 • edited Loading

kwasniew left a comment

Choose a reason for hiding this comment

kwasniew Oct 9, 2023

Choose a reason for hiding this comment

sighphyre Oct 12, 2023

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

sighphyre commented Oct 12, 2023

Tymek Oct 24, 2023

Choose a reason for hiding this comment

sighphyre Oct 24, 2023

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

Tymek commented Oct 5, 2023 •

edited

Loading

coveralls commented Oct 5, 2023 •

edited

Loading