Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
60 changes: 47 additions & 13 deletions oss_lib/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,25 +119,43 @@ def register_arguments(parser, service_prefix):
parser.add_argument(
"--debug",
action="store_true",
default=False,
help="Set the logging level to DEBUG, overrides {0}_DEBUG "
"environment variable. (default: false).".format(service_prefix),
default=None,
help="Sets the logging level to DEBUG, overrides the {0}_DEBUG "
"environment variable and configuration "
"(possible values: true/false, others - unset)."
.format(service_prefix),
)
parser.add_argument(
"--no-debug",
action="store_false",
dest="debug",
default=None,
help="Opposite to --debug, disables the debug mode and overrides "
"the environment variable and a configuration value.",
)
return parser


def get_debug_value(env_value):
if env_value == "true":
return True
elif env_value == "false":
return False
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks strange, how debug could be non-True and non-False in the same time.
Why not simply decide if debug is True or False, right here?

Also, just recommendation: consider checking "0" and "1", "False" and "True" as well.
Another approach is strict check - if env_value is non-empty and neither "true" nor "false" then raise an error with message that env variable is set to strange value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The debug environment variable can be set and it can have correct value, such as "true" and "false", incorrect value, or can be not set at all. Also, the default value is determined later in the code.

However, I agree that a validation have to be done in this function instead of silently hide all incorrect values and return None as though they are not set. I will fix it in the next patches.



def get_env_variables(service_prefix):
debug = os.environ.get(service_prefix + "_DEBUG") == "true"
debug = os.environ.get(service_prefix + "_DEBUG")
variables = {
"config_path": os.environ.get(service_prefix + "_CONF"),
"log_config_path": os.environ.get(service_prefix + "_LOG_CONF"),
"debug": debug,
"debug": get_debug_value(debug),
}
return variables


def setup_config(config_path=None, default_config_path=None, defaults=None,
log_config_path=None, debug=False,
log_config_path=None, debug=None,
validation_schema=None):
global CONF, _CONF

Expand All @@ -152,10 +170,16 @@ def setup_config(config_path=None, default_config_path=None, defaults=None,
if loaded_config:
merge_dicts(config, loaded_config)

config.setdefault("debug", False)
if debug is not None:
config["debug"] = debug

if debug:
log_config(config, config_path, default_config_path)

validate_config(config, validation_schema)
validate_config(config,
defaults=defaults,
validation_schema=validation_schema)
_CONF = config
return CONF

Expand Down Expand Up @@ -214,23 +238,33 @@ def merge_dicts(source, other):
def log_config(config, config_path, default_config_path):
LOG.debug("Configuration files: config_path=%s, default_config_path=%s",
config_path, default_config_path)
LOG.debug("Content of the configuration:")
LOG.debug("Resulting configuration content:")
content = io.BytesIO()
yaml.dump(config, stream=content, default_flow_style=False)
content.seek(0)
for line in content:
LOG.debug(line)
LOG.debug(line.rstrip())
LOG.debug("End of resulting configuration content.")


def get_required_properties(defaults, schema):
if defaults:
return list(set(schema.keys()) - set(defaults.keys()))
return list(schema.keys())


def validate_config(config, validation_schema=None):
def validate_config(config, defaults=None, validation_schema=None):
schema = {
"type": "object",
"$schema": "http://json-schema.org/draft-04/schema",
"properties": {},
"properties": {
"debug": {"type": "boolean"},
},
"additionalProperties": False,
}
# TODO(akscram): Generate the `required` key based on passed
# defaults and validation_schema properties.
if validation_schema:
schema["properties"].update(validation_schema)
required = get_required_properties(defaults, validation_schema)
if required:
schema["required"] = required
jsonschema.validate(config, schema)
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

setup(
name="oss_lib",
version="0.1.1",
version="0.1.2",
description="OSS Tooling Library",
license="Apache 2.0",
url="https://github.com/seecloud/oss-lib",
Expand Down
31 changes: 29 additions & 2 deletions tests/unit/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,12 @@ def test_register_arguments(self):
help=mock.ANY),
mock.call("--debug",
action="store_true",
default=False,
default=None,
help=mock.ANY),
mock.call("--no-debug",
action="store_false",
dest="debug",
default=None,
help=mock.ANY),
]

Expand All @@ -77,7 +82,7 @@ def test_register_arguments(self):
DEFAULT_VARS = {
"config_path": None,
"log_config_path": None,
"debug": False,
"debug": None,
}

@ddt.data(
Expand Down Expand Up @@ -180,11 +185,33 @@ def test_merge_dicts(self, one, other, result):
config.merge_dicts(one, other)
assert one == result

@ddt.data(
({}, {}, []),
(None, {}, []),
({}, {"a": {"type": "integer"}}, ["a"]),
({"a": 1}, {"a": {"type": "integer"}}, []),
(
{"a": 1},
{"a": {"type": "integer"}, "b": {"type": "string"}},
["b"],
),
)
@ddt.unpack
def test_get_required_properties(self, defaults, schema, result):
value = config.get_required_properties(defaults, schema)
assert value == result

@ddt.data(
({}, None, False),
({"a": 1234}, None, True),
({"a": 1234}, {"a": {"type": "integer"}}, False),
({"a": 1234}, {"a": {"type": "string"}}, True),
({}, {"b": {"type": "string"}}, True),
(
{"a": 1234},
{"a": {"type": "integer"}, "b": {"type": "string"}},
True,
),
)
@ddt.unpack
def test_validate_config(self, conf, schema, error):
Expand Down