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

Small tweaks to simplify the code #325

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Small tweaks to simplify the code #325

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
daae8c7
zaza/utilities/deployment_env.py: simplify is_valid_env_key
Jan 29, 2020
1df4ac7
zaza/utilities/deployment_env.py: simplify find_setup_file
Jan 29, 2020
07dc850
zaza/utilities/deployment_env.py: simplify get_setup_file_contents
Jan 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 10 additions & 18 deletions zaza/utilities/deployment_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
import os
import functools
import yaml
from os.path import expandvars

ZAZA_SETUP_FILE_LOCATIONS = [
'{home}/.zaza.yaml']
expandvars('$HOME/.zaza.yaml'),
]
Comment on lines 23 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the same way as line 35 change, this could also be a tuple.

However, I would leave out the expandvars() call here, and delay it to where the string is used. This is because it's just about possible that the HOME env will change between load time of the module and use time of the function.


SECRETS = 'secrets'
RUNTIME_CONFIG = 'runtime_config'
Expand All @@ -32,14 +34,14 @@
MODEL_SETTINGS_SECTION = 'model_settings'
MODEL_CONSTRAINTS_SECTION = 'model_constraints'

VALID_ENVIRONMENT_KEY_PREFIXES = [
VALID_ENVIRONMENT_KEY_PREFIXES = (
'OS_',
'TEST_',
'MOJO_',
'JUJU_',
'CHARM_',
'MODEL_',
]
)

MODEL_DEFAULTS = {
# Model defaults from charm-test-infra
Expand Down Expand Up @@ -116,12 +118,7 @@ def is_valid_env_key(key):
:returns: Whether key is a valid environment variable name
:rtype: bool
"""
valid = False
for _k in VALID_ENVIRONMENT_KEY_PREFIXES:
if key.startswith(_k):
valid = True
break
return valid
return key.startswith(VALID_ENVIRONMENT_KEY_PREFIXES)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice optimisation; might be useful to add a comment indicating that .startswith() can also take the tuple of values to match against (which I didn't know about! - TIL!)



def find_setup_file():
Expand All @@ -130,11 +127,9 @@ def find_setup_file():
:returns: Location of zaza config file or None if not found.
:rtype: str or None
"""
ctxt = {
'home': os.environ.get('HOME')}
for setup_file in ZAZA_SETUP_FILE_LOCATIONS:
if os.path.isfile(setup_file.format(**ctxt)):
return setup_file.format(**ctxt)
if os.path.isfile(setup_file):
return setup_file
Comment on lines +131 to +132
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do the expandvars() call here to be consistent with the current semantics of the function; i.e. evaluate the HOME env in this function rather than at module load time.



def get_setup_file_contents():
Expand All @@ -145,16 +140,13 @@ def get_setup_file_contents():
:rtype: dict
"""
setup_file = find_setup_file()
setup_file_data = {}
if setup_file:
with open(setup_file, 'r') as stream:
try:
_file_data = yaml.safe_load(stream)
if _file_data:
setup_file_data.update(_file_data)
return yaml.safe_load(stream) or {}
except yaml.YAMLError:
logging.warn("Unable to load data from {}".format(setup_file))
return setup_file_data
return {}


def get_setup_file_section(section_name):
Expand Down