Merge lp:~gfidente/cloud-init/try_fqdn_from_hosts into lp:~cloud-init-dev/cloud-init/trunk

[Giulio Fidente]

In util.py , get_hostname_fqdn tries cloud.get_hostname(fqdn=True) to get the fqdn, but that might return just the hostname if fqdn isn't found.

This change is meant to make it try get_fqdn_from_hosts if fqdn does not contain any dot.

[Joshua Harlow (harlowja)]

Anyway to add some unittests for this?

[Giulio Fidente (gfidente)]

Yes absolutely, thanks for checking this out. There wasn't any for the hostname functions from util.py so I will be adding a bunch.

[lp:~gfidente/cloud-init/try_fqdn_from_hosts]

1220. By Giulio Fidente on 2016-05-13

Adds unit tests for util.py

This adds seven new unit tests are meant to cover get_hostname_fqdn() and
get_fqdn_from_hosts() from util.py

1221. By Giulio Fidente on 2016-05-13

Fix test_from_cloud()

The FakeCloudWithDomain provider was incorrectly returning the fqdn
when called with fqdn=False

[Giulio Fidente (gfidente)]

Do you think it would be useful to add the unit tests first, without any additional code change to diminish the code changes related to the new logic into a different proposal?

[Giulio Fidente (gfidente)]

hi, thanks again for reviewing this, will do!

[lp:~gfidente/cloud-init/try_fqdn_from_hosts]

1222. By Giulio Fidente on 2016-05-18

Ensure temporary 'hosts' file used for testing is deleted via addCleanup

While tearDown is not executed in case of errors raised during the setUp
phase while addCleanup is; this change updates test_util.py so that the
temporary 'hosts' file created to run some unit tests is deleted in
addCleanup instead of tearDown.

[Juan Antonio Osorio Robles (juan-osorio-robles)]

would sure use this fix O_O. The fix LGTM

[Scott Moser (smoser)]

Hello,
Thank you for taking the time to contribute to cloud-init. Cloud-init has moved its revision control system to git. As a result, we are marking all bzr merge proposals as 'rejected'. If you would like to re-submit this proposal for review, please do so by following the current HACKING documentation at http://cloudinit.readthedocs.io/en/latest/topics/hacking.html .

Please do re-submit.
Sorry for lost time and unresponsiveness.

Scott

Unmerged revisions

1222. By Giulio Fidente on 2016-05-18

Ensure temporary 'hosts' file used for testing is deleted via addCleanup

While tearDown is not executed in case of errors raised during the setUp
phase while addCleanup is; this change updates test_util.py so that the
temporary 'hosts' file created to run some unit tests is deleted in
addCleanup instead of tearDown.

1221. By Giulio Fidente on 2016-05-13

Fix test_from_cloud()

The FakeCloudWithDomain provider was incorrectly returning the fqdn
when called with fqdn=False

1220. By Giulio Fidente on 2016-05-13

Adds unit tests for util.py

This adds seven new unit tests are meant to cover get_hostname_fqdn() and
get_fqdn_from_hosts() from util.py

1219. By Giulio Fidente on 2016-05-13

Ensure fqdn is not left to None if get_fqdn_from_hosts returns None

1218. By Giulio Fidente on 2016-05-13

Try get_fqdn_from_hosts if get_hostname(fqdn=True) is unable to find one

=== modified file 'cloudinit/util.py'
--- cloudinit/util.py	2016-05-12 17:56:26 +0000
+++ cloudinit/util.py	2016-05-18 22:38:52 +0000
@@ -933,7 +933,7 @@
     return contents.replace('\r\n', '\n')
 
 
-def get_hostname_fqdn(cfg, cloud):
+def get_hostname_fqdn(cfg, cloud, hosts_filename='/etc/hosts'):
     # return the hostname and fqdn from 'cfg'.  If not found in cfg,
     # then fall back to data from cloud
     if "fqdn" in cfg:
@@ -947,13 +947,17 @@
             fqdn = cfg['hostname']
             hostname = cfg['hostname'][:fqdn.find('.')]
         else:
+            if "hostname" in cfg:
+                hostname = cfg['hostname']
+            else:
+                hostname = cloud.get_hostname()
             # no fqdn set, get fqdn from cloud.
             # get hostname from cfg if available otherwise cloud
             fqdn = cloud.get_hostname(fqdn=True)
-            if "hostname" in cfg:
-                hostname = cfg['hostname']
-            else:
-                hostname = cloud.get_hostname()
+            if fqdn.find('.') < 1:
+                fqdn_from_hosts = get_fqdn_from_hosts(hostname, hosts_filename)
+                if fqdn_from_hosts:
+                    fqdn = fqdn_from_hosts
     return (hostname, fqdn)
 
 
 
=== modified file 'tests/unittests/test_util.py'
--- tests/unittests/test_util.py	2016-03-14 13:45:11 +0000
+++ tests/unittests/test_util.py	2016-05-18 22:38:52 +0000
@@ -37,6 +37,20 @@
         self.restored.append(path)
 
 
+class FakeCloudWithoutDomain(object):
+
+    def get_hostname(self, fqdn=False):
+        return "cloudname"
+
+
+class FakeCloudWithDomain(object):
+
+    def get_hostname(self, fqdn=False):
+        if fqdn:
+            return "cloudname.clouddomain"
+        return "cloudname"
+
+
 class TestGetCfgOptionListOrStr(helpers.TestCase):
     def test_not_found_no_default(self):
         """None is returned if key is not found and no default given."""
@@ -69,6 +83,79 @@
         self.assertEqual([], result)
 
 
+class TestGetHostnameFqdn(helpers.TestCase):
+    def setUp(self):
+        super(TestGetHostnameFqdn, self).setUp()
+        tmp_h, self.tmp_path = tempfile.mkstemp()
+        self.addCleanup(os.remove, self.tmp_path)
+        self.f = os.fdopen(tmp_h, "w")
+        self.f.write("9.10.11.12 cloudname.clouddomain cloudname\n")
+        self.f.close()
+        self.cloud = FakeCloudWithDomain()
+        self.cloud_no_domain = FakeCloudWithoutDomain()
+
+    def test_with_fqdn(self):
+        """Results based only on fqdn config key"""
+        cfg = {'fqdn': 'thisname.thisdomain'}
+        hostname, fqdn = util.get_hostname_fqdn(cfg, self.cloud, self.tmp_path)
+        self.assertEqual("thisname", hostname)
+        self.assertEqual("thisname.thisdomain", fqdn)
+
+    def test_with_dotted_hostname(self):
+        """Results based only on hostname config key"""
+        cfg = {'hostname': 'thisname.thisdomain'}
+        hostname, fqdn = util.get_hostname_fqdn(cfg, self.cloud, self.tmp_path)
+        self.assertEqual("thisname", hostname)
+        self.assertEqual("thisname.thisdomain", fqdn)
+
+    def test_with_short_hostname(self):
+        """The hostname is from config key, the fqdn from cloud"""
+        cfg = {'hostname': 'thisname'}
+        hostname, fqdn = util.get_hostname_fqdn(cfg, self.cloud, self.tmp_path)
+        self.assertEqual("thisname", hostname)
+        self.assertEqual("cloudname.clouddomain", fqdn)
+
+    def test_from_cloud(self):
+        """Results based only on cloud"""
+        cfg = {}
+        hostname, fqdn = util.get_hostname_fqdn(cfg, self.cloud, self.tmp_path)
+        self.assertEqual("cloudname", hostname)
+        self.assertEqual("cloudname.clouddomain", fqdn)
+
+    def test_from_cloud_without_domain(self):
+        """The hostname is from cloud, the fqdn mapped from hosts file"""
+        cfg = {}
+        hostname, fqdn = util.get_hostname_fqdn(cfg,
+                                                self.cloud_no_domain,
+                                                self.tmp_path)
+        self.assertEqual("cloudname", hostname)
+        self.assertEqual("cloudname.clouddomain", fqdn)
+
+
+class TestGetFqdnFromHosts(helpers.TestCase):
+    def setUp(self):
+        super(TestGetFqdnFromHosts, self).setUp()
+        tmp_h, self.tmp_path = tempfile.mkstemp()
+        self.addCleanup(os.remove, self.tmp_path)
+        self.f = os.fdopen(tmp_h, "w")
+        self.f.write("# comment\n")
+        self.f.write("1.2.3.4 name.domain name\n")
+        self.f.write("5.6.7.8 myname.mydomain myname # comment\n")
+        self.f.close()
+
+    def test_fqdn_found(self):
+        """Test fqdn is returned when hostname is found"""
+        hostname = "myname"
+        fqdn = util.get_fqdn_from_hosts(hostname, self.tmp_path)
+        self.assertEqual("myname.mydomain", fqdn)
+
+    def test_fqdn_not_found(self):
+        """Test None is returned when hostname is not found"""
+        hostname = "unnamed"
+        fqdn = util.get_fqdn_from_hosts(hostname, self.tmp_path)
+        self.assertEqual(None, fqdn)
+
+
 class TestWriteFile(helpers.TestCase):
     def setUp(self):
         super(TestWriteFile, self).setUp()