Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix get bool env Issue #102 #103

Closed
wants to merge 5 commits into from
Closed

Fix get bool env Issue #102 #103

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bcce13a
fixed bug in get_bool_env. https://github.com/pryorda/vmware_exporter…
May 9, 2019
5a46d09
added additional test case to test get_bool_env when an env variable …
May 9, 2019
eedb481
added additional test case to test get_bool_env when an env variable …
May 9, 2019
04ca419
modified get_bool_env to work with both upper case and lower case val…
May 10, 2019
28daae6
Merge branch 'master' into fix_get_bool_env
cybervedaa May 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion tests/unit/test_helpers.py
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ def test_get_bool_env_with_default_value():
assert value


def test_get_bool_env_with_a_valid_env():
def test_get_bool_env_with_a_valid_env_true():
key = "TEST_BOOLEAN_VALUE"

os.environ[key] = "True"
Expand All @@ -32,6 +32,16 @@ def test_get_bool_env_with_a_valid_env():
assert value


def test_get_bool_env_with_a_valid_env_false():
key = "TEST_BOOLEAN_VALUE"

os.environ[key] = "False"

value = get_bool_env(key, True)

assert value is False


def test_batch_fetch_properties():
content = mock.Mock()

Expand Down
8 changes: 7 additions & 1 deletion vmware_exporter/helpers.py
View file
Open in desktop
Expand Up @@ -4,7 +4,13 @@


def get_bool_env(key, default=None):
return bool(os.environ.get(key, default))
value = os.environ.get(key, default)
if isinstance(value, str):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats about:

if value is None:
  return default
else:
  return value.lower() == 'true'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kremers - it gets complicated, because get_bool_env is passed values of type bool for the parameterdefault in some sections of the code. E.g. this snippet under VMWareMetricsResource.configure

'ignore_ssl': get_bool_env('VSPHERE_IGNORE_SSL', False),
                'collect_only': {
                    'vms': get_bool_env('VSPHERE_COLLECT_VMS', True),
                    'vmguests': get_bool_env('VSPHERE_COLLECT_VMGUESTS', True),
                    'datastores': get_bool_env('VSPHERE_COLLECT_DATASTORES', True),
                    'hosts': get_bool_env('VSPHERE_COLLECT_HOSTS', True),
                    'snapshots': get_bool_env('VSPHERE_COLLECT_SNAPSHOTS', True),

so we end up having to deal with str, bool and None. Therefore the check for isinstance(value, str)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

def get_bool_env(key: str, default: bool):
  value = os.environ.get(key, default)
  return value if type(value) == bool else value.lower() == 'true'

or longer syntax

def get_bool_env(key: str, default: bool):
  value = os.environ.get(key, default)
  if type(value) == bool:
    return value
  else:
    return value.lower() == 'true'

Does handle all cases? 'None' could never appear since all calls to get_bool_env, get parametrized with bool values, so I would vote for removing it as default value. Not sure if type(....) == bool is more neat than isinstance(value, bool).

if value == 'False':
pryorda marked this conversation as resolved.
Show resolved Hide resolved
value = False
elif value == 'True':
value = True
return value


def batch_fetch_properties(content, obj_type, properties):
Expand Down