dcos-865 add duplicate validation for package.sources

Also don't save changes if it creates more errors in append, prepend,
set and unset.

cli/dcoscli/config/main.py

@@ -22,6 +22,7 @@ Positional Arguments:
 """
 
 import collections
+import copy
 import json
 import os
 
@@ -58,6 +59,36 @@ def main():
     return returncode
 
 
+def compare_validations(toml_config_pre, toml_config_post):
+    """
+    :param toml_config_pre: dictionary for the value before change
+    :type toml_config_pre: dcos.api.config.Toml
+    :param toml_config_post: dictionary for the value with change
+    :type toml_config_post: dcos.api.config.Toml
+    :returns: process status
+    :rtype: int
+    """
+
+    errors_pre = util.validate_json(toml_config_pre._dictionary,
+                                    _generate_root_schema(toml_config_pre))
+    errors_post = util.validate_json(toml_config_post._dictionary,
+                                     _generate_root_schema(toml_config_post))
+    if len(errors_post) != 0:
+        if len(errors_pre) == 0:
+            emitter.publish(util.list_to_err(errors_post))
+            return 1
+
+        def _errs(errs):
+            return set([e.split('\n')[0] for e in errs])
+
+        diff_errors = _errs(errors_post) - _errs(errors_pre)
+        if len(diff_errors) != 0:
+            emitter.publish(util.list_to_err(errors_post))
+            return 1
+
+    return 0
+
+
 def _cmds():
     """
     :returns: all the supported commands
@@ -134,6 +165,9 @@ def _set(name, value):
         emitter.publish(err)
         return 1
 
+    toml_config_pre = copy.deepcopy(toml_config)
+    if section not in toml_config_pre._dictionary:
+        toml_config_pre._dictionary[section] = {}
     toml_config[name] = python_value
 
     if (name == 'core.reporting' and python_value is True) or \
@@ -142,6 +176,10 @@ def _set(name, value):
 
     _save_config_file(config_path, toml_config)
 
+    if compare_validations(toml_config_pre, toml_config) == 1:
+        return 1
+
+    _save_config_file(config_path, toml_config)
     return 0
 
 
@@ -157,10 +195,16 @@ def _append(name, value):
     if err is not None:
         emitter.publish(err)
         return 1
-
+    toml_config_pre = copy.deepcopy(toml_config)
+    section = name.split(".", 1)[0]
+    if section not in toml_config_pre._dictionary:
+        toml_config_pre._dictionary[section] = {}
     toml_config[name] = toml_config.get(name, []) + python_value
-    _save_config_file(config_path, toml_config)
 
+    if compare_validations(toml_config_pre, toml_config) == 1:
+        return 1
+
+    _save_config_file(config_path, toml_config)
     return 0
 
 
@@ -177,9 +221,15 @@ def _prepend(name, value):
         emitter.publish(err)
         return 1
 
+    toml_config_pre = copy.deepcopy(toml_config)
+    section = name.split(".", 1)[0]
+    if section not in toml_config_pre._dictionary:
+        toml_config_pre._dictionary[section] = {}
     toml_config[name] = python_value + toml_config.get(name, [])
-    _save_config_file(config_path, toml_config)
+    if compare_validations(toml_config_pre, toml_config) == 1:
+        return 1
 
+    _save_config_file(config_path, toml_config)
     return 0
 
 
@@ -190,7 +240,10 @@ def _unset(name, index):
     """
 
     config_path, toml_config = _load_config()
-
+    toml_config_pre = copy.deepcopy(toml_config)
+    section = name.split(".", 1)[0]
+    if section not in toml_config_pre._dictionary:
+        toml_config_pre._dictionary[section] = {}
     value = toml_config.pop(name, None)
     if value is None:
         emitter.publish(
@@ -223,8 +276,10 @@ def _unset(name, index):
                 'Unsetting based on an index is only supported for lists'))
         return 1
 
-    _save_config_file(config_path, toml_config)
+    if compare_validations(toml_config_pre, toml_config) == 1:
+        return 1
 
+    _save_config_file(config_path, toml_config)
     return 0
 
 
@@ -264,6 +319,23 @@ def _validate():
 
     _, toml_config = _load_config()
 
+    errs = util.validate_json(toml_config._dictionary,
+                              _generate_root_schema(toml_config))
+    if len(errs) != 0:
+        emitter.publish(util.list_to_err(errs))
+        return 1
+
+    return 0
+
+
+def _generate_root_schema(toml_config):
+    """
+    :param toml_configs: dictionary of values
+    :type toml_config: TomlConfig
+    :returns: configuration_schema
+    :rtype: jsonschema
+    """
+
     root_schema = {
         '$schema': 'http://json-schema.org/schema#',
         'type': 'object',
@@ -280,12 +352,7 @@ def _validate():
 
         root_schema['properties'][section] = config_schema
 
-    err = util.validate_json(toml_config._dictionary, root_schema)
-    if err is not None:
-        emitter.publish(err)
-        return 1
-
-    return 0
+    return root_schema
 
 
 def _generate_choice_msg(name, value):

cli/dcoscli/data/config-schema/package.json

@@ -10,7 +10,8 @@
       "title": "Package sources",
       "description": "The list of package source in search order",
       "default": [ "git://github.com/mesosphere/universe.git" ],
-      "additionalItems": false
+      "additionalItems": false,
+      "uniqueItems": true
     },
     "cache": {
       "type": "string",

cli/dcoscli/marathon/main.py

@@ -242,9 +242,9 @@ def _add(app_resource):
             'dcoscli',
             'data/marathon-schema.json').decode('utf-8'))
 
-    err = util.validate_json(application_resource, schema)
-    if err is not None:
-        emitter.publish(err)
+    errs = util.validate_json(application_resource, schema)
+    if len(errs) != 0:
+        emitter.publish(util.list_to_err(errs))
         return 1
 
     # Add application to marathon
@@ -387,9 +387,9 @@ def _start(app_id, instances, force):
 
     app_json['instances'] = instances
 
-    err = util.validate_json(app_json, schema)
-    if err is not None:
-        emitter.publish(err)
+    errs = util.validate_json(app_json, schema)
+    if len(errs) != 0:
+        emitter.publish(util.list_to_err(errs))
         return 1
 
     deployment, err = client.update_app(app_id, app_json, force)
@@ -494,9 +494,9 @@ def _update(app_id, json_items, force):
         else:
             app_json[key] = value
 
-    err = util.validate_json(app_json, schema)
-    if err is not None:
-        emitter.publish(err)
+    errs = util.validate_json(app_json, schema)
+    if len(errs) != 0:
+        emitter.publish(util.list_to_err(errs))
         return 1
 
     deployment, err = client.update_app(app_id, app_json, force)

cli/tests/data/missing_params_dcos.toml

@@ -0,0 +1,7 @@
+[core]
+reporting = false
+email = "test@mail.com"
+[marathon]
+[package]
+sources = [ "git://github.com/mesosphere/universe.git", "https://github.com/mesosphere/universe/archive/master.zip",]
+cache = "true"

cli/tests/integrations/cli/test_config.py

@@ -18,6 +18,15 @@ def env():
     }
 
 
+@pytest.fixture
+def missing_env():
+    return {
+        constants.PATH_ENV: os.environ[constants.PATH_ENV],
+        constants.DCOS_CONFIG_ENV:
+            os.path.join("tests", "data", "missing_params_dcos.toml")
+    }
+
+
 def test_help():
     stdout = b"""Get and set DCOS CLI configuration properties
 
@@ -109,7 +118,7 @@ def test_set_existing_integral_property(env):
 
 
 def test_append_empty_list(env):
-    _unset_value('package.sources', None, env)
+    _set_value('package.sources', '[]', env)
     _append_value(
         'package.sources',
         'git://github.com/mesosphere/universe.git',
@@ -130,7 +139,7 @@ def test_append_empty_list(env):
 
 
 def test_prepend_empty_list(env):
-    _unset_value('package.sources', None, env)
+    _set_value('package.sources', '[]', env)
     _prepend_value(
         'package.sources',
         'https://github.com/mesosphere/universe/archive/master.zip',
@@ -201,9 +210,9 @@ def test_prepend_non_list(env):
 
 
 def test_unset_property(env):
-    _unset_value('marathon.host', None, env)
-    _get_missing_value('marathon.host', env)
-    _set_value('marathon.host', 'localhost', env)
+    _unset_value('core.reporting', None, env)
+    _get_missing_value('core.reporting', env)
+    _set_value('core.reporting', 'false', env)
 
 
 def test_unset_missing_property(env):
@@ -214,6 +223,14 @@ def test_unset_missing_property(env):
         env=env)
 
 
+def test_set_whole_list(env):
+    _set_value(
+        'package.sources',
+        '["git://github.com/mesosphere/universe.git", '
+        '"https://github.com/mesosphere/universe/archive/master.zip"]',
+        env)
+
+
 def test_unset_top_property(env):
     stderr = (
         b"Property 'marathon' doesn't fully specify a value - "
@@ -229,16 +246,6 @@ def test_unset_top_property(env):
         env=env)
 
 
-def test_unset_whole_list(env):
-    _unset_value('package.sources', None, env)
-    _get_missing_value('package.sources', env)
-    _set_value(
-        'package.sources',
-        '["git://github.com/mesosphere/universe.git", '
-        '"https://github.com/mesosphere/universe/archive/master.zip"]',
-        env)
-
-
 def test_unset_list_index(env):
     _unset_value('package.sources', '0', env)
     _get_value(
@@ -290,13 +297,11 @@ def test_validate(env):
 
 
 def test_validation_error(env):
-    _unset_value('marathon.host', None, env)
-
-    stderr = b"""\
-Error: missing required property 'host'. \
+    stderr = b"""Error: missing required property 'host'. \
 Add to JSON file and pass in /path/to/file with the --options argument.
 """
-    assert_command(['dcos', 'config', 'validate'],
+
+    assert_command(['dcos', 'config', 'unset', 'marathon.host'],
                    returncode=1,
                    stderr=stderr,
                    env=env)
@@ -312,10 +317,11 @@ def test_set_property_key(env):
         env=env)
 
 
-def test_set_missing_property(env):
-    _unset_value('marathon.host', None, env)
-    _set_value('marathon.host', 'localhost', env)
-    _get_value('marathon.host', 'localhost', env)
+def test_set_missing_property(missing_env):
+    _set_value('marathon.host', 'localhost', missing_env)
+    _get_value('marathon.host', 'localhost', missing_env)
+    _set_value('marathon.port', '8080', missing_env)
+    _get_value('marathon.port', '8080', missing_env)
 
 
 def test_set_core_property(env):

dcos/api/package.py

@@ -1163,9 +1163,9 @@ class Package():
         logger.info('Merged options: %r', options)
 
         # Validate options with the config schema
-        err = util.validate_json(options, config_schema)
-        if err is not None:
-            return (None, err)
+        errs = util.validate_json(options, config_schema)
+        if len(errs) != 0:
+            return (None, util.list_to_err(errs))
 
         return (options, None)
 

dcos/api/util.py

@@ -234,8 +234,8 @@ def validate_json(instance, schema):
     :type instance: dict
     :param schema: the schema to validate with
     :type schema: dict
-    :returns: an error if the validation failed; None otherwise
-    :rtype: Error
+    :returns: list of errors as strings
+    :rtype: list
     """
 
     # TODO: clean up this hack
@@ -272,13 +272,20 @@ def validate_json(instance, schema):
         message += 'Value: {}'.format(json.dumps(error.instance))
         return message
 
-    formatted_errors = [format(e) for e in validation_errors]
+    return [format(e) for e in validation_errors]
 
-    if len(formatted_errors) is 0:
-        return None
-    else:
-        errors_as_str = str.join('\n\n', formatted_errors)
-        return errors.DefaultError(errors_as_str)
+
+def list_to_err(errs):
+    """convert list of errors to Error
+
+    :param errors: list of string errors
+    :type errors: list of strings
+    :returns: error message
+    :rtype: Error
+    """
+
+    errs_as_str = str.join('\n\n', errs)
+    return errors.DefaultError(errs_as_str)
 
 
 def parse_int(string):