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.

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

Fix get bool env Issue #102 #103

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
4 participants
@cybervedaa
Copy link

commented May 9, 2019

Contains fix for #102

Vedavyas Kadudas added some commits May 9, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 9, 2019

Codecov Report

Merging #103 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #103     +/-   ##
=========================================
+ Coverage   92.32%   92.43%   +0.1%     
=========================================
  Files           4        4             
  Lines         482      489      +7     
=========================================
+ Hits          445      452      +7     
  Misses         37       37
Impacted Files Coverage Δ
vmware_exporter/helpers.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19135a6...28daae6. Read the comment docs.

@pryorda
Copy link
Owner

left a comment

This looks good to me let me know your thoughts on .lower()?

Show resolved Hide resolved vmware_exporter/helpers.py Outdated
@cybervedaa

This comment has been minimized.

Copy link
Author

commented May 10, 2019

Vedavyas Kadudas Vedavyas Kadudas
@kremers

This comment has been minimized.

Copy link

commented on vmware_exporter/helpers.py in bcce13a May 15, 2019

I don't think it can be any different from str or None

https://docs.python.org/3/library/os.html#os.environ

@@ -4,7 +4,14 @@


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

This comment has been minimized.

Copy link
@kremers

kremers May 15, 2019

Contributor

Whats about:

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

This comment has been minimized.

Copy link
@cybervedaa

cybervedaa May 15, 2019

Author

@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)

This comment has been minimized.

Copy link
@kremers

kremers May 16, 2019

Contributor
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).

@pryorda

This comment has been minimized.

Copy link
Owner

commented May 17, 2019

Closing in favor of #108

@pryorda pryorda closed this May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.