From d940985432af67ac8397c2239606a52fea400db3 Mon Sep 17 00:00:00 2001 From: Scott K Logan Date: Wed, 17 Apr 2024 17:15:09 -0500 Subject: [PATCH 1/6] Add central interface for defining feature flags The intended use for colcon feature flags is to ship pre-production and prototype features in a disabled state, which can be enabled by specifying a particular environment variable value. By using an environment variable, these possibly dangerous or unstable features are hidden from common users but are enabled in a way which can be audited. --- colcon_core/feature_flags.py | 45 +++++++++++++++++++++++ test/spell_check.words | 4 ++ test/test_feature_flags.py | 71 ++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+) create mode 100644 colcon_core/feature_flags.py create mode 100644 test/test_feature_flags.py diff --git a/colcon_core/feature_flags.py b/colcon_core/feature_flags.py new file mode 100644 index 00000000..a93bab7e --- /dev/null +++ b/colcon_core/feature_flags.py @@ -0,0 +1,45 @@ +# Copyright 2024 Open Source Robotics Foundation, Inc. +# Licensed under the Apache License, Version 2.0 + +import os +import re + +from colcon_core.environment_variable import EnvironmentVariable + + +"""Environment variable to enable feature flags""" +FEATURE_FLAGS_ENVIRONMENT_VARIABLE = EnvironmentVariable( + 'COLCON_FEATURE_FLAGS', + 'Enable pre-production features and behaviors') + + +def get_feature_flags(): + """ + Retrieve all enabled feature flags. + + :returns: List of enabled flags + :rtype: list + """ + return [ + flag for flag in ( + os.environ.get(FEATURE_FLAGS_ENVIRONMENT_VARIABLE.name) or '' + ).split(os.pathsep) if flag + ] + + +def is_feature_flag_set(flag): + """ + Determine if a specific feature flag is enabled. + + Feature flags are case-sensitive and separated by the os-specific path + separator character. + + :param str flag: Name of the flag to search for + + :returns: True if the flag is set + :rtype: bool + """ + return bool(flag and re.search( + fr'(?:^|{os.pathsep}){flag}(?:{os.pathsep}|$)', + os.environ.get(FEATURE_FLAGS_ENVIRONMENT_VARIABLE.name) or '', + )) diff --git a/test/spell_check.words b/test/spell_check.words index 83548464..772114a9 100644 --- a/test/spell_check.words +++ b/test/spell_check.words @@ -1,3 +1,4 @@ +addfinalizer addopts apache argparse @@ -35,8 +36,10 @@ docstring executables exitstatus fdopen +ffoo filterwarnings foobar +fooo fromhex functools getcategory @@ -139,5 +142,6 @@ unittest unittests unlinking unrenamed +usefixtures wildcards workaround diff --git a/test/test_feature_flags.py b/test/test_feature_flags.py new file mode 100644 index 00000000..f6ba8d8d --- /dev/null +++ b/test/test_feature_flags.py @@ -0,0 +1,71 @@ +# Copyright 2024 Open Source Robotics Foundation, Inc. +# Licensed under the Apache License, Version 2.0 + +import os +from unittest.mock import patch + +from colcon_core.feature_flags import FEATURE_FLAGS_ENVIRONMENT_VARIABLE +from colcon_core.feature_flags import get_feature_flags +from colcon_core.feature_flags import is_feature_flag_set +import pytest + + +_FLAGS_TO_TEST = ( + ('foo',), + ('foo', 'foo'), + ('foo', ''), + ('', 'foo'), + ('', 'foo', ''), + ('foo', 'bar'), + ('bar', 'foo'), + ('bar', 'foo', 'baz'), +) + + +@pytest.fixture +def feature_flags_value(request): + env = dict(os.environ) + if request.param is not None: + env[FEATURE_FLAGS_ENVIRONMENT_VARIABLE.name] = os.pathsep.join( + request.param) + else: + env.pop(FEATURE_FLAGS_ENVIRONMENT_VARIABLE.name, None) + + mock_env = patch('colcon_core.feature_flags.os.environ', env) + request.addfinalizer(mock_env.stop) + mock_env.start() + return request.param + + +@pytest.mark.parametrize( + 'feature_flags_value', + _FLAGS_TO_TEST, + indirect=True) +@pytest.mark.usefixtures('feature_flags_value') +def test_flag_is_set(): + assert is_feature_flag_set('foo') + + +@pytest.mark.parametrize( + 'feature_flags_value', + (None, *_FLAGS_TO_TEST), + indirect=True) +@pytest.mark.usefixtures('feature_flags_value') +def test_flag_not_set(): + assert not is_feature_flag_set('') + assert not is_feature_flag_set('fo') + assert not is_feature_flag_set('oo') + assert not is_feature_flag_set('fooo') + assert not is_feature_flag_set('ffoo') + assert not is_feature_flag_set('qux') + + +@pytest.mark.parametrize( + 'feature_flags_value', + (None, *_FLAGS_TO_TEST), + indirect=True) +@pytest.mark.usefixtures('feature_flags_value') +def test_get_flags(feature_flags_value): + assert [ + flag for flag in (feature_flags_value or ()) if flag + ] == get_feature_flags() From 545b4c74c1fab8771ab2c3ffc50b983d49286d27 Mon Sep 17 00:00:00 2001 From: Scott K Logan Date: Fri, 19 Apr 2024 14:48:52 -0500 Subject: [PATCH 2/6] Switch from regex to string.split --- colcon_core/feature_flags.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/colcon_core/feature_flags.py b/colcon_core/feature_flags.py index a93bab7e..896af69d 100644 --- a/colcon_core/feature_flags.py +++ b/colcon_core/feature_flags.py @@ -2,7 +2,6 @@ # Licensed under the Apache License, Version 2.0 import os -import re from colcon_core.environment_variable import EnvironmentVariable @@ -39,7 +38,4 @@ def is_feature_flag_set(flag): :returns: True if the flag is set :rtype: bool """ - return bool(flag and re.search( - fr'(?:^|{os.pathsep}){flag}(?:{os.pathsep}|$)', - os.environ.get(FEATURE_FLAGS_ENVIRONMENT_VARIABLE.name) or '', - )) + return flag in get_feature_flags() From 5c309fa58715789fd69a55b4342a897954fd8f30 Mon Sep 17 00:00:00 2001 From: Scott K Logan Date: Fri, 19 Apr 2024 14:56:57 -0500 Subject: [PATCH 3/6] Log a message when an enabled feature is queried --- colcon_core/feature_flags.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/colcon_core/feature_flags.py b/colcon_core/feature_flags.py index 896af69d..1eb4224d 100644 --- a/colcon_core/feature_flags.py +++ b/colcon_core/feature_flags.py @@ -4,13 +4,17 @@ import os from colcon_core.environment_variable import EnvironmentVariable +from colcon_core.logging import colcon_logger +logger = colcon_logger.getChild(__name__) """Environment variable to enable feature flags""" FEATURE_FLAGS_ENVIRONMENT_VARIABLE = EnvironmentVariable( 'COLCON_FEATURE_FLAGS', 'Enable pre-production features and behaviors') +_REPORTED_USES = set() + def get_feature_flags(): """ @@ -38,4 +42,9 @@ def is_feature_flag_set(flag): :returns: True if the flag is set :rtype: bool """ - return flag in get_feature_flags() + if flag in get_feature_flags(): + if flag not in _REPORTED_USES: + logger.warning(f'Enabling feature: {flag}') + _REPORTED_USES.add(flag) + return True + return False From 6566fa58d048b5ad6bca9a356165cf333d8199d9 Mon Sep 17 00:00:00 2001 From: Scott K Logan Date: Mon, 20 May 2024 16:11:44 -0500 Subject: [PATCH 4/6] Add list of 'implemented' feature flags and warning --- colcon_core/command.py | 4 +++ colcon_core/feature_flags.py | 22 ++++++++++++++- test/test_entry_point.py | 2 ++ test/test_feature_flags.py | 53 ++++++++++++++++++++++++++++++------ 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/colcon_core/command.py b/colcon_core/command.py index 228d3b36..c6721ff9 100644 --- a/colcon_core/command.py +++ b/colcon_core/command.py @@ -52,6 +52,7 @@ from colcon_core.argument_parser import decorate_argument_parser # noqa: E402 E501 I100 I202 from colcon_core.argument_parser import SuppressUsageOutput # noqa: E402 from colcon_core.extension_point import load_extension_points # noqa: E402 +from colcon_core.feature_flags import check_implemented_flags # noqa: E402 from colcon_core.location import create_log_path # noqa: E402 from colcon_core.location import get_log_path # noqa: E402 from colcon_core.location import set_default_config_path # noqa: E402 @@ -135,6 +136,9 @@ def _main(*, command_name, argv): 'Command line arguments: {argv}' .format(argv=argv if argv is not None else sys.argv)) + # warn about any specified feature flags that are already implemented + check_implemented_flags() + # set default locations for config files, for searchability: COLCON_HOME set_default_config_path( path=(Path('~') / f'.{command_name}').expanduser(), diff --git a/colcon_core/feature_flags.py b/colcon_core/feature_flags.py index 1eb4224d..7eef1f14 100644 --- a/colcon_core/feature_flags.py +++ b/colcon_core/feature_flags.py @@ -15,6 +15,19 @@ _REPORTED_USES = set() +IMPLEMENTED_FLAGS = set() + + +def check_implemented_flags(): + """Check for and warn about flags which have been implemented.""" + implemented = IMPLEMENTED_FLAGS.intersection(get_feature_flags()) + if implemented: + logger.warning( + 'The following feature flags have been implemented and should no ' + 'longer be specified in ' + f'{FEATURE_FLAGS_ENVIRONMENT_VARIABLE.name}: ' + f"{','.join(implemented)}") + def get_feature_flags(): """ @@ -42,8 +55,15 @@ def is_feature_flag_set(flag): :returns: True if the flag is set :rtype: bool """ - if flag in get_feature_flags(): + if flag in IMPLEMENTED_FLAGS: + return True + elif flag in get_feature_flags(): if flag not in _REPORTED_USES: + if not _REPORTED_USES: + logger.warning( + 'One or more feature flags have been enabled. ' + 'These features may be unstable and may change API or ' + 'behavior at any time.') logger.warning(f'Enabling feature: {flag}') _REPORTED_USES.add(flag) return True diff --git a/test/test_entry_point.py b/test/test_entry_point.py index bbb797b1..aae8d920 100644 --- a/test/test_entry_point.py +++ b/test/test_entry_point.py @@ -9,6 +9,8 @@ with warnings.catch_warnings(): warnings.filterwarnings( 'ignore', message='.*entry_point.*deprecated.*', category=UserWarning) + warnings.filterwarnings( + 'ignore', message='.*pkg_resources.*', category=DeprecationWarning) from colcon_core.entry_point import EXTENSION_POINT_GROUP_NAME from colcon_core.entry_point import get_all_entry_points diff --git a/test/test_feature_flags.py b/test/test_feature_flags.py index f6ba8d8d..218b2977 100644 --- a/test/test_feature_flags.py +++ b/test/test_feature_flags.py @@ -4,6 +4,7 @@ import os from unittest.mock import patch +from colcon_core.feature_flags import check_implemented_flags from colcon_core.feature_flags import FEATURE_FLAGS_ENVIRONMENT_VARIABLE from colcon_core.feature_flags import get_feature_flags from colcon_core.feature_flags import is_feature_flag_set @@ -37,27 +38,41 @@ def feature_flags_value(request): return request.param +@pytest.fixture +def feature_flag_reports(request): + reported_uses = patch('colcon_core.feature_flags._REPORTED_USES', set()) + request.addfinalizer(reported_uses.stop) + reported_uses.start() + return reported_uses + + @pytest.mark.parametrize( 'feature_flags_value', _FLAGS_TO_TEST, indirect=True) -@pytest.mark.usefixtures('feature_flags_value') +@pytest.mark.usefixtures('feature_flags_value', 'feature_flag_reports') def test_flag_is_set(): - assert is_feature_flag_set('foo') + with patch('colcon_core.feature_flags.logger.warning') as warn: + assert is_feature_flag_set('foo') + assert warn.call_count == 2 + assert is_feature_flag_set('foo') + assert warn.call_count == 2 @pytest.mark.parametrize( 'feature_flags_value', (None, *_FLAGS_TO_TEST), indirect=True) -@pytest.mark.usefixtures('feature_flags_value') +@pytest.mark.usefixtures('feature_flags_value', 'feature_flag_reports') def test_flag_not_set(): - assert not is_feature_flag_set('') - assert not is_feature_flag_set('fo') - assert not is_feature_flag_set('oo') - assert not is_feature_flag_set('fooo') - assert not is_feature_flag_set('ffoo') - assert not is_feature_flag_set('qux') + with patch('colcon_core.feature_flags.logger.warning') as warn: + assert not is_feature_flag_set('') + assert not is_feature_flag_set('fo') + assert not is_feature_flag_set('oo') + assert not is_feature_flag_set('fooo') + assert not is_feature_flag_set('ffoo') + assert not is_feature_flag_set('qux') + assert warn.call_count == 0 @pytest.mark.parametrize( @@ -69,3 +84,23 @@ def test_get_flags(feature_flags_value): assert [ flag for flag in (feature_flags_value or ()) if flag ] == get_feature_flags() + + +@pytest.mark.parametrize('feature_flags_value', (('baz',),), indirect=True) +@pytest.mark.usefixtures('feature_flags_value') +def test_implemented(): + with patch('colcon_core.feature_flags.IMPLEMENTED_FLAGS', {'foo'}): + with patch('colcon_core.feature_flags.logger.warning') as warn: + assert not is_feature_flag_set('bar') + assert warn.call_count == 0 + assert is_feature_flag_set('baz') + assert warn.call_count == 2 + assert is_feature_flag_set('foo') + assert warn.call_count == 2 + check_implemented_flags() + assert warn.call_count == 2 + + with patch('colcon_core.feature_flags.IMPLEMENTED_FLAGS', {'baz'}): + with patch('colcon_core.feature_flags.logger.warning') as warn: + check_implemented_flags() + assert warn.call_count == 1 From c58ea45665b3f4555b644409e4b59b66da52df8b Mon Sep 17 00:00:00 2001 From: Scott K Logan Date: Tue, 21 May 2024 18:32:58 -0700 Subject: [PATCH 5/6] Note the env var in the the warning message --- colcon_core/feature_flags.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/colcon_core/feature_flags.py b/colcon_core/feature_flags.py index 7eef1f14..56bb5bec 100644 --- a/colcon_core/feature_flags.py +++ b/colcon_core/feature_flags.py @@ -61,9 +61,10 @@ def is_feature_flag_set(flag): if flag not in _REPORTED_USES: if not _REPORTED_USES: logger.warning( - 'One or more feature flags have been enabled. ' - 'These features may be unstable and may change API or ' - 'behavior at any time.') + 'One or more feature flags have been enabled using the ' + f'{FEATURE_FLAGS_ENVIRONMENT_VARIABLE.name} environment ' + 'variable. These features may be unstable and may change ' + 'API or behavior at any time.') logger.warning(f'Enabling feature: {flag}') _REPORTED_USES.add(flag) return True From 360024cc53f35c19e3bd17076c1925ac48450992 Mon Sep 17 00:00:00 2001 From: Scott K Logan Date: Fri, 31 May 2024 16:17:26 -0500 Subject: [PATCH 6/6] Revert an inadvertant change --- test/test_entry_point.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/test_entry_point.py b/test/test_entry_point.py index aae8d920..bbb797b1 100644 --- a/test/test_entry_point.py +++ b/test/test_entry_point.py @@ -9,8 +9,6 @@ with warnings.catch_warnings(): warnings.filterwarnings( 'ignore', message='.*entry_point.*deprecated.*', category=UserWarning) - warnings.filterwarnings( - 'ignore', message='.*pkg_resources.*', category=DeprecationWarning) from colcon_core.entry_point import EXTENSION_POINT_GROUP_NAME from colcon_core.entry_point import get_all_entry_points