ServiceManager: fix variables override

Given the following configuration:
variables:
    foo: bar

services:
    simple:
        desc: Simplest service declaration
        actions:
            status:
                cmd: echo %foo

We have this behaviour:
$ milkcheck -c ~/devel/milkcheck-cedeyn/conf/test_variables status --var 'foo=bar' -d
[14:36:42] WARNING  - Configuration file /etc/milkcheck/milkcheck.conf not found
[14:36:42] DEBUG    - Configuration
assumeyes: False
config_dir:
/ccc/home/cont001/ocre/cedeyna/devel/milkcheck-cedeyn/conf/test_variables
confirm_actions: []
defines: ['foo=bar']
dryrun: False
fanout: 64
nodeps: False
report: no
reverse_actions: ['stop']
summary: False
tags: set([])
verbosity: 5
[14:36:42] ERROR    -

We hit the VariableAlreadyDefined exception.
Re-ordering and updating variables fixes this bug.

Also add test to check if milkcheck correctly override variables given on its
commandline.

Change-Id: I80ca2be6b85bd9fc2de89c00e40054956f529e4b

lib/MilkCheck/Engine/BaseEntity.py

@@ -322,6 +322,15 @@ def remove_var(self, varname):
         if varname in self.variables:
             del self.variables[varname]
 
+    def update_var(self, varname, value):
+        """ Update existing variable """
+        # Debugging
+        logger = logging.getLogger('milkcheck')
+        logger.info("Variable '%s' updating '%s' (was '%s')",
+                    varname, value, self.variables[varname])
+        self.remove_var(varname)
+        self.add_var(varname, value)
+
     def update_target(self, nodeset, mode=None):
         '''Update the attribute target of an entity'''
         assert nodeset is not None

lib/MilkCheck/ServiceManager.py

@@ -33,7 +33,7 @@
 This module contains the ServiceManager class definition.
 '''
 
-from MilkCheck.Engine.BaseEntity import LOCKED, WARNING
+from MilkCheck.Engine.BaseEntity import LOCKED, WARNING, VariableAlreadyExistError
 from MilkCheck.Engine.ServiceGroup import ServiceGroup, ServiceNotFoundError
 
 
@@ -76,7 +76,10 @@ def _variable_config(self, conf):
             for defines in conf.get('defines', []):
                 for define in defines.split():
                     key, value = define.split('=', 1)
-                    self.add_var(key, value)
+                    try:
+                        self.add_var(key, value)
+                    except VariableAlreadyExistError:
+                        self.update_var(key, value)
         else:
             for varname in ('selected_node', 'excluded_nodes'):
                 self.add_var(varname.upper(), '')
@@ -148,14 +151,15 @@ def call_services(self, services, action, conf=None):
         self.reset()
         self.variables.clear()
 
-        # Create global variable from configuration
-        self._variable_config(conf)
         if conf:
             # Apply configuration over the graph
             self._apply_config(conf)
             # Enable reverse mode if needed, based on config
             self.algo_reversed = action in conf.get('reverse_actions')
 
+        # Create global variable from configuration
+        self._variable_config(conf)
+
         # Ensure all variables have been resolved
         self.resolve_all()
 

tests/MilkCheckTests/UITests/CliTest.py

@@ -1,5 +1,6 @@
-# Copyright CEA (2011-2017)
+# Copyright CEA (2011-2019)
 # Contributor: TATIBOUET Jeremie
+# Contributor: CEDEYN Aurelien
 #
 
 """
@@ -913,6 +914,7 @@ def test_command_output_warning_status(self):
         self._output_check(['warn', 'go', '-q'], RC_WARNING,
 """warn                                                              [ WARNING ]
 """)
+
     def test_custom_defines(self):
         '''Test command line output custom variables'''
         svc = Service('one')
@@ -922,6 +924,28 @@ def test_custom_defines(self):
 """go one on localhost
  > /bin/echo bar
 one                                                               [    OK   ]
+""")
+
+    def test_overriding_defines(self):
+        """Test command line with overriden variables"""
+        tmpdir = tempfile.mkdtemp(prefix='test-mlk-')
+        tmpfile = tempfile.NamedTemporaryFile(suffix='.yaml', dir=tmpdir)
+        tmpfile.write(textwrap.dedent("""
+            variables:
+                foo: pub
+
+            services:
+              svc:
+                actions:
+                  start:
+                    cmd: echo %foo
+                    """))
+        tmpfile.flush()
+
+        self._output_check(['svc', 'start', '-v', '-c', tmpdir, '--define', 'foo=bar'], RC_OK,
+"""start svc on localhost
+ > echo bar
+svc                                                               [    OK   ]
 """)
 
 class CLIConfigDirTests(CLICommon):