Merge pull request #486 from darko-poljak/dependency_warn

Add warning message for faulty dependencies case.

cdist/conf/type/__firewalld_start/man.rst

@@ -1,5 +1,5 @@
 cdist-type__firewalld_start(7)
-=============================
+==============================
 
 NAME
 ----

cdist/emulator.py

@@ -84,6 +84,10 @@ class Emulator(object):
         self.type_name = os.path.basename(argv[0])
         self.cdist_type = core.CdistType(self.type_base_path, self.type_name)
 
+        # If set then object alreay exists and this var holds existing
+        # requirements.
+        self._existing_reqs = None
+
         self.__init_log()
 
     def run(self):
@@ -166,6 +170,9 @@ class Emulator(object):
                 self.parameters[key] = value
 
         if self.cdist_object.exists and 'CDIST_OVERRIDE' not in self.env:
+            # Make existing requirements a set so that we can compare it
+            # later with new requirements.
+            self._existing_reqs = set(self.cdist_object.requirements)
             if self.cdist_object.parameters != self.parameters:
                 errmsg = ("Object %s already exists with conflicting "
                           "parameters:\n%s: %s\n%s: %s" % (
@@ -243,7 +250,7 @@ class Emulator(object):
         return cdist_object.name
 
     def record_requirements(self):
-        """record requirements"""
+        """Record requirements."""
 
         # Inject the predecessor, but not if its an override
         # (this would leed to an circular dependency)
@@ -267,6 +274,7 @@ class Emulator(object):
                     # so do not set a requirement
                     pass
 
+        reqs = set()
         if "require" in self.env:
             requirements = self.env['require']
             self.log.debug("reqs = " + requirements)
@@ -274,7 +282,23 @@ class Emulator(object):
                 # Ignore empty fields - probably the only field anyway
                 if len(requirement) == 0:
                     continue
-                self.record_requirement(requirement)
+                object_name = self.record_requirement(requirement)
+                reqs.add(object_name)
+        if self._existing_reqs is not None:
+            # If object exists then compare existing and new requirements.
+            if self._existing_reqs != reqs:
+                warnmsg = ("Object {} already exists with requirements:\n"
+                           "{}: {}\n"
+                           "{}: {}\n"
+                           "Dependency resolver could not handle dependencies "
+                           "as expected.".format(
+                               self.cdist_object.name,
+                               " ".join(self.cdist_object.source),
+                               self._existing_reqs,
+                               self.object_source,
+                               reqs
+                           ))
+                self.log.warning(warnmsg)
 
     def record_auto_requirements(self):
         """An object shall automatically depend on all objects that it

cdist/test/config/__init__.py

@@ -177,6 +177,22 @@ class ConfigRunTestCase(test.CdistTestCase):
         dryrun.run()
         # if we are here, dryrun works like expected
 
+    def test_desp_resolver(self):
+        """Test to show dependency resolver warning message."""
+        local = cdist.exec.local.Local(
+            target_host=self.target_host,
+            base_root_path=self.host_base_path,
+            host_dir_name=self.hostdir,
+            exec_path=os.path.abspath(os.path.join(
+                my_dir, '../../../scripts/cdist')),
+            initial_manifest=os.path.join(
+                fixtures, 'manifest/init-deps-resolver'),
+            add_conf_dirs=[fixtures])
+
+        # dry_run is ok for dependency testing
+        config = cdist.config.Config(local, self.remote, dry_run=True)
+        config.run()
+
 
 # Currently the resolving code will simply detect that this object does
 # not exist. It should probably check if the type is a singleton as well

cdist/test/config/fixtures/manifest/init-deps-resolver

@@ -0,0 +1,8 @@
+__a a
+require="__e/e" __b b
+require="__f/f" __c c
+__e e
+__f f
+require="__c/c" __d d
+__g g
+__h h

cdist/test/config/fixtures/type/__g/manifest

@@ -0,0 +1 @@
+require="__c/c __d/d" __a a

cdist/test/config/fixtures/type/__h/manifest

@@ -0,0 +1,3 @@
+# require="__b/b" __a a
+require="__j/j" __i i
+__j j

cdist/test/emulator/__init__.py

@@ -499,6 +499,60 @@ class StdinTestCase(test.CdistTestCase):
         self.assertEqual(random_string, stdin_saved_by_emulator)
 
 
+class EmulatorAlreadyExistingRequirementsWarnTestCase(test.CdistTestCase):
+
+    def setUp(self):
+        self.temp_dir = self.mkdtemp()
+        handle, self.script = self.mkstemp(dir=self.temp_dir)
+        os.close(handle)
+        base_path = self.temp_dir
+        hostdir = cdist.str_hash(self.target_host[0])
+        host_base_path = os.path.join(base_path, hostdir)
+
+        self.local = local.Local(
+            target_host=self.target_host,
+            base_root_path=host_base_path,
+            host_dir_name=hostdir,
+            exec_path=test.cdist_exec_path,
+            add_conf_dirs=[conf_dir])
+        self.local.create_files_dirs()
+
+        self.manifest = core.Manifest(self.target_host, self.local)
+        self.env = self.manifest.env_initial_manifest(self.script)
+        self.env['__cdist_object_marker'] = self.local.object_marker_name
+
+    def tearDown(self):
+        shutil.rmtree(self.temp_dir)
+
+    def test_object_existing_requirements_req_none(self):
+        """Test to show dependency resolver warning message."""
+        argv = ['__directory', 'spam']
+        emu = emulator.Emulator(argv, env=self.env)
+        emu.run()
+        argv = ['__file', 'eggs']
+        self.env['require'] = '__directory/spam'
+        emu = emulator.Emulator(argv, env=self.env)
+        emu.run()
+        argv = ['__file', 'eggs']
+        if 'require' in self.env:
+            del self.env['require']
+        emu = emulator.Emulator(argv, env=self.env)
+
+    def test_object_existing_requirements_none_req(self):
+        """Test to show dependency resolver warning message."""
+        argv = ['__directory', 'spam']
+        emu = emulator.Emulator(argv, env=self.env)
+        emu.run()
+        argv = ['__file', 'eggs']
+        if 'require' in self.env:
+            del self.env['require']
+        emu = emulator.Emulator(argv, env=self.env)
+        emu.run()
+        argv = ['__file', 'eggs']
+        self.env['require'] = '__directory/spam'
+        emu = emulator.Emulator(argv, env=self.env)
+
+
 if __name__ == '__main__':
     import unittest
     unittest.main()

docs/src/man1/cdist.rst

@@ -246,6 +246,35 @@ connection. In this case ssh will disable multiplexing.
 This limit is controlled with sshd :strong:`MaxSessions` configuration
 options. For more details refer to :strong:`sshd_config`\ (5).
 
+When requirements for the same object are defined in different manifests (see
+example below) in init manifest and in some other type manifest and they differs
+then dependency resolver cannot detect dependencies right. This happens because
+cdist cannot prepare all objects first and then run objects because some
+object can depend on the result of type explorer(s) and explorers are executed
+during object run. cdist will detect such case and write warning message.
+Example for such a case:
+
+.. code-block:: sh
+
+    init manifest:
+        __a a
+        require="__e/e" __b b
+        require="__f/f" __c c
+        __e e
+        __f f
+        require="__c/c" __d d
+        __g g
+        __h h
+
+    type __g manifest:
+        require="__c/c __d/d" __a a
+
+    Warning message:
+        WARNING: cdisttesthost: Object __a/a already exists with requirements:
+        /usr/home/darko/ungleich/cdist/cdist/test/config/fixtures/manifest/init-deps-resolver /tmp/tmp.cdist.test.ozagkg54/local/759547ff4356de6e3d9e08522b0d0807/data/conf/type/__g/manifest: set()
+        /tmp/tmp.cdist.test.ozagkg54/local/759547ff4356de6e3d9e08522b0d0807/data/conf/type/__g/manifest: {'__c/c', '__d/d'}
+        Dependency resolver could not handle dependencies as expected.
+
 COPYING
 -------
 Copyright \(C) 2011-2013 Nico Schottelius. Free use of this software is