docs: feature flag handling #217

MaximilianSoerenPollak · 2025-01-24T10:21:23Z

This PR is still a WIP as there is tests missing and some implementation details may change.

Any feedback is appreciated.

Todo

PR provides a way to disable requirements based on feature flags set in bazel.
It has a 'translation' layer where we can map feature flags to tags, which are then used to disable/hide requirements.

Closes #202

docs/_tooling/sphinx_extensions/sphinx_extensions/build/modularity/README.md

docs/_tooling/sphinx_extensions/sphinx_extensions/build/modularity/modularity.py

This PR provdies a way to disable requirements based on feature flags set in bazel. It has a 'translation' layer where we can map feature flags to tags, which are then used to disable/hide requirements.

Added integration tests to the packaging. Added docstrings to functions inside the extension. Formatting also done.

docs/_tooling/sphinx_extensions/sphinx_extensions/build/modularity/README.md

LittleHuba · 2025-01-28T12:35:34Z

docs/_tooling/sphinx_extensions/sphinx_extensions/build/modularity/README.md

+```bzl
+FEATURE_TAG_MAPPING = {
+    "feature1": ["some-ip", "tag2"], # --//docs:feature1=true -> will expand to 'some-ip', 'tag2'
+    "second-feature": ["test-feat", "tag6"],
+}
+```
+
+The default values are defined like such: 
+```bzl
+def define_feature_flags(name):
+    bool_flag(
+        name = "feature1",
+        build_setting_default = False,
+    )
+    bool_flag(
+        name = "second-feature",
+        build_setting_default = False,
+    )
+
+    feature_flag_translator(
+        name = name,
+        flags = {":feature1": "True", ":second-feature": "True"},
+    )
+```


This restricts us to only bool flags. We also need to consider other feature_flag types of Skylark. But for now this is a great start.

That is true, but the foundation at least should allow this to be expanded into other flag types in the future. The translation then might have to be adapted to account for those.

Revised logic, so now feature=true means that those requirements are enabled, not disabled. Added a decision record to lay out why 'hide' was choosen Adapted README to make it a bit clearer. Added wrappers for filtering functions used by needpie, etc.

AlexanderLanin · 2025-01-29T18:57:52Z

docs/BUILD

@@ -171,3 +177,28 @@ score_py_pytest(
    visibility = ["//visibility:public"],
    deps = [":score_metamodel"],
 )
+
+py_library(
+    name = "modularity",


Can we pick a name that is less ambiguous? "modularity" is too close to "module" which we will be using for the SCORE code implementations. Something with "variant", "feature" or "flag" maybe?

The name was chosen as packaging didn't sound right and might interfere with other things. I think for sure though a more descriptive name would be better.
Maybe one of these?

FlagConfiguration

VariantHandling

FeatureFlagsConfig

ConfigureRequirements

FeatureFlaggedRequirements 😆

docs/_tooling/extensions/modularity/README.md

AlexanderLanin · 2025-01-29T20:57:32Z

docs/_tooling/extensions/modularity/README.md

+   This requirement will not be disabled.
+```
+We can then build it with our feature flag enabled via `bazel build //docs:docs --//docs:second-feature=true` 
+This will expand via our translation layer `feature_flag.bzl` into the tags `test-feat` and `tag5`.  


Why do we need such a translation layer? Unless I missed it, it's not described anywhere?

The main reason why it is needed is to make it possible to expand one flag into multiple tags. We theoretically could work without this layer with some adaptations, but then you need to add EACH tag as a separate argument in the command.

Edit:
It also allows for more complex 'flags' in the future instead of just 'boolean' as it is currently.

Why do we have more flags than commands? Can you mention that somewhere in md/rst?

docs/_tooling/extensions/modularity/README.md

docs/_tooling/extensions/modularity/__init__.py

AlexanderLanin · 2025-01-29T21:17:50Z

docs/_tooling/extensions/modularity/tests/test_modularity.py

+    fake_app_ok = SimpleNamespace()
+    fake_app_ok.config = SimpleNamespace()
+    fake_app_ok.config.filter_tags_file_path = str(
+        feature_flags_dir / "filter_tags.txt"
+    )


e.g.: fake_app_ok = fake_app({filter_tags_file_path: str( feature_flags_dir / "filter_tags.txt" )})

AlexanderLanin · 2025-01-29T21:20:40Z

docs/_tooling/extensions/modularity/tests/test_modularity.py

+def test_modularity_hide_ok(
+    sphinx_app_setup, basic_conf, basic_rst_file, positive_filter_tags, sphinx_base_dir
+):
+    app = sphinx_app_setup(basic_conf, basic_rst_file, positive_filter_tags)


docs/_tooling/extensions/modularity/tests/test_modularity.py

docs/_tooling/requirements_lock.txt

docs/conf.py

Reworked README to add clarity Renamed extension from modularity to give a more meaningful name Reverted upgraded lockfile Integrated extension/feature into 'incremental'

danwos · 2025-02-06T08:50:16Z

Can't the extension score_feature_flag_handling be a contribution to Sphinx-Needs itself?

As some Sphinx-Needs internal functions get monkey patched, I see a huge risk that this may not work with upcoming Sphinx-Needs releases and it keeps the maintenance high on the SCORE side.

So would it be okay to create an upstream PR with this and get rid of the SCORE-specific implementation for the Sphinx-Needs part?
For sure the discussion in Sphinx-Needs may take some time, so the SCORE-specific solution is still needed during this time.

MaximilianSoerenPollak · 2025-02-06T10:10:43Z

Can't the extension score_feature_flag_handling be a contribution to Sphinx-Needs itself?

As some Sphinx-Needs internal functions get monkey patched, I see a huge risk that this may not work with upcoming Sphinx-Needs releases and it keeps the maintenance high on the SCORE side.

So would it be okay to create an upstream PR with this and get rid of the SCORE-specific implementation for the Sphinx-Needs part? For sure the discussion in Sphinx-Needs may take some time, so the SCORE-specific solution is still needed during this time.

This for sure would make future compatibility nicer and easier. I could make this an upstream PR, although some questions would need to be clarified etc.
Should we make an issue in the Sphinx-needs repo to discuss this further?

chrisjsewell · 2025-02-06T13:42:03Z

docs/_tooling/extensions/score_feature_flag_handling/__init__.py

+def setup(app):
+    logger.debug("score_feature_flag_handling extension loaded")
+    app.add_config_value("filter_tags_file_path", None, "env")
+    app.connect("env-updated", hide_needs)


There is a specific event for this needs-before-sealing; func(app, needs), as mentioned in https://sphinx-needs.readthedocs.io/en/latest/changelog.html#improvements-to-filtering-at-scale

Hmm interesting, I must have made a mistake then. I tried this in the beginning but decided not go go with it as something didn't quite work. I will check it out again, as if we have such specific events it would be nicer to call them correctly.
Thanks for pointing it out.

danwos · 2025-02-06T13:54:01Z

An issue in the Sphinx-Needs repo would be super.
I will also support as much as I can.

danwos · 2025-02-07T06:35:44Z

I have created a related Sphinx-Needs ticket: useblocks/sphinx-needs#1399

@MaximilianSoerenPollak Can you please check, if my description/idea supports your use case or if anything is missing? Thanks.

AlexanderLanin · 2025-02-07T07:43:14Z

@danwos how about a small issue for sphinx-needs to correctly account for hide in tables, pies etc.? That should be trivially implementable without further discussions?

danwos · 2025-02-26T08:37:16Z

@danwos how about a small issue for sphinx-needs to correctly account for hide in tables, pies etc.? That should be trivially implementable without further discussions?

That's tough because the current implementation is the expected one.
hide shall hide it, but it shall be part of the internal needs dictionary and also be part of all filtering. Some projects are using it this way.

For "hiding" it completely, delete was introduced, but it does not support dynamic functions and variants support right now. So this needs to be fixed somehow.

AlexanderLanin · 2025-02-26T09:22:03Z

@danwos well it can be there. It should just not show up in needpie etc?!

danwos · 2025-02-26T12:06:04Z

That's the point for hide, it shall be part of needpie and co. But it shall not have its own representation in the documentation.

AlexanderLanin reviewed Jan 27, 2025

View reviewed changes

MaximilianSoerenPollak commented Jan 28, 2025

View reviewed changes

docs/_tooling/sphinx_extensions/sphinx_extensions/build/modularity/modularity.py Outdated Show resolved Hide resolved

MaximilianSoerenPollak added 2 commits January 28, 2025 09:32

docs: Added ability to disable requirements

4c0b2e2

This PR provdies a way to disable requirements based on feature flags set in bazel. It has a 'translation' layer where we can map feature flags to tags, which are then used to disable/hide requirements.

docs: Added tests to packaging

bb8671f

Added integration tests to the packaging. Added docstrings to functions inside the extension. Formatting also done.

LittleHuba reviewed Jan 28, 2025

View reviewed changes

AlexanderLanin added community:infrastructure General Score infrastructure topics docs-as-code labels Jan 28, 2025

docs: Configuring of requirements

6ddf262

Revised logic, so now feature=true means that those requirements are enabled, not disabled. Added a decision record to lay out why 'hide' was choosen Adapted README to make it a bit clearer. Added wrappers for filtering functions used by needpie, etc.

MaximilianSoerenPollak force-pushed the MaximilianSoerenPollak-packaging-concept branch from e1fa9ab to 6ddf262 Compare January 29, 2025 16:55

AlexanderLanin reviewed Jan 29, 2025

View reviewed changes

docs: Feature Flag handling

a234a0f

Reworked README to add clarity Renamed extension from modularity to give a more meaningful name Reverted upgraded lockfile Integrated extension/feature into 'incremental'

MaximilianSoerenPollak marked this pull request as ready for review January 30, 2025 10:43

MaximilianSoerenPollak changed the title ~~docs: WIP packaging concept~~ docs: feature flag handling Jan 31, 2025

chrisjsewell reviewed Feb 6, 2025

View reviewed changes

danwos mentioned this pull request Feb 7, 2025

Build tag to remove/delete needs useblocks/sphinx-needs#1399

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: feature flag handling #217

docs: feature flag handling #217

MaximilianSoerenPollak commented Jan 24, 2025 •

edited

Loading

LittleHuba Jan 28, 2025

MaximilianSoerenPollak Jan 30, 2025

AlexanderLanin Jan 29, 2025

MaximilianSoerenPollak Jan 30, 2025

AlexanderLanin Jan 30, 2025

AlexanderLanin Jan 29, 2025

MaximilianSoerenPollak Jan 30, 2025 •

edited

Loading

AlexanderLanin Jan 30, 2025

AlexanderLanin Jan 29, 2025

AlexanderLanin Jan 29, 2025

danwos commented Feb 6, 2025

MaximilianSoerenPollak commented Feb 6, 2025

chrisjsewell Feb 6, 2025

MaximilianSoerenPollak Feb 6, 2025

danwos commented Feb 6, 2025

danwos commented Feb 7, 2025 •

edited

Loading

AlexanderLanin commented Feb 7, 2025

danwos commented Feb 26, 2025

AlexanderLanin commented Feb 26, 2025

danwos commented Feb 26, 2025

docs: feature flag handling #217

Are you sure you want to change the base?

docs: feature flag handling #217

Conversation

MaximilianSoerenPollak commented Jan 24, 2025 • edited Loading

This PR is still a WIP as there is tests missing and some implementation details may change.

Any feedback is appreciated.

Todo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximilianSoerenPollak Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwos commented Feb 6, 2025

MaximilianSoerenPollak commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwos commented Feb 6, 2025

danwos commented Feb 7, 2025 • edited Loading

AlexanderLanin commented Feb 7, 2025

danwos commented Feb 26, 2025

AlexanderLanin commented Feb 26, 2025

danwos commented Feb 26, 2025

MaximilianSoerenPollak commented Jan 24, 2025 •

edited

Loading

MaximilianSoerenPollak Jan 30, 2025 •

edited

Loading

danwos commented Feb 7, 2025 •

edited

Loading