hostname: Make strategies consistent, drop py2.4

Change:
- Make strategies behave consistently and return the empty string
  instead of "UNKNOWN" (or "temporarystub") for the "before" value if
  the permanent hostname file does not exist or could not be read.
- Switch to `with open()` instead of annoying exception handling code
  (which was wrong and leaked file handles in several places). This
  drops Python 2.4 support for this module.
- Updated porting guide since users could be relying on these former,
  inconsistent values.

Signed-off-by: Rick Elrod <rick@elrod.me>

changelogs/fragments/66432_hostname_check_mode_writes.yml

@@ -1,2 +1,7 @@
 bugfixes:
-  - hostname - No longer modifies system files in get_* methods consulted when in check_mode (https://github.com/ansible/ansible/issues/66432)
+  - hostname - No longer modifies system files in get_* methods and therefore when consulted in check_mode (https://github.com/ansible/ansible/issues/66432)
+
+breaking_changes:
+  - hostname - Drops any remaining support for Python 2.4 by using ``with open()`` to simplify exception handling code which leaked file handles in several spots
+  - hostname - On FreeBSD, the string ``temporarystub`` no longer gets written to the hostname file in the get methods (and in check_mode). As a result, the default hostname will now appear as ``''`` (empty string) instead of ``temporarystub`` for consistency with other strategies. This means the ``before`` result will be different.
+  - hostname - On OpenRC systems and Solaris, the ``before`` value will now be ``''`` (empty string) if the permanent hostname file does not exist, for consistency with other strategies.

docs/docsite/rst/porting_guides/porting_guide_core_2.12.rst

@@ -52,6 +52,8 @@ Modules
 
 * ``cron`` now requires ``name`` to be specified in all cases.
 * ``cron`` no longer allows a ``reboot`` parameter. Use ``special_time: reboot`` instead.
+* ``hostname`` - On FreeBSD, the ``before`` result will no longer be ``"temporarystub"`` if permanent hostname file does not exist. It will instead be ``""`` (empty string) for consistency with other systems.
+* ``hostname`` - On OpenRC and Solaris based systems, the ``before`` result will no longer be ``"UNKNOWN"`` if the permanent hostname file does not exist. It will instead be ``""`` (empty string) for consistency with other systems.
 
 
 Modules removed

lib/ansible/modules/hostname.py

@@ -240,25 +240,21 @@ class DebianStrategy(GenericStrategy):
             return ''
 
         try:
-            f = open(self.HOSTNAME_FILE)
+            with open(self.HOSTNAME_FILE, 'r') as f:
-            try:
                 return f.read().strip()
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to read hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to read hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
     def set_permanent_hostname(self, name):
         try:
-            f = open(self.HOSTNAME_FILE, 'w+')
+            with open(self.HOSTNAME_FILE, 'w+') as f:
-            try:
                 f.write("%s\n" % name)
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to update hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to update hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
 
 class SLESStrategy(GenericStrategy):
@@ -273,25 +269,21 @@ class SLESStrategy(GenericStrategy):
             return ''
 
         try:
-            f = open(self.HOSTNAME_FILE)
+            with open(self.HOSTNAME_FILE) as f:
-            try:
                 return f.read().strip()
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to read hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to read hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
     def set_permanent_hostname(self, name):
         try:
-            f = open(self.HOSTNAME_FILE, 'w+')
+            with open(self.HOSTNAME_FILE, 'w+') as f:
-            try:
                 f.write("%s\n" % name)
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to update hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to update hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
 
 class RedHatStrategy(GenericStrategy):
@@ -303,42 +295,35 @@ class RedHatStrategy(GenericStrategy):
 
     def get_permanent_hostname(self):
         try:
-            f = open(self.NETWORK_FILE, 'rb')
+            with open(self.NETWORK_FILE, 'rb') as f:
-            try:
                 for line in f.readlines():
                     if line.startswith('HOSTNAME'):
                         k, v = line.split('=')
                         return v.strip()
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to read hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to read hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
     def set_permanent_hostname(self, name):
         try:
             lines = []
             found = False
-            f = open(self.NETWORK_FILE, 'rb')
+            with open(self.NETWORK_FILE, 'rb') as f:
-            try:
                 for line in f.readlines():
                     if line.startswith('HOSTNAME'):
                         lines.append("HOSTNAME=%s\n" % name)
                         found = True
                     else:
                         lines.append(line)
-            finally:
-                f.close()
             if not found:
                 lines.append("HOSTNAME=%s\n" % name)
-            f = open(self.NETWORK_FILE, 'w+')
+            with open(self.NETWORK_FILE, 'w+') as f:
-            try:
                 f.writelines(lines)
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to update hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to update hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
 
 class AlpineStrategy(GenericStrategy):
@@ -359,25 +344,21 @@ class AlpineStrategy(GenericStrategy):
             return ''
 
         try:
-            f = open(self.HOSTNAME_FILE)
+            with open(self.HOSTNAME_FILE) as f:
-            try:
                 return f.read().strip()
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to read hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to read hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
     def set_permanent_hostname(self, name):
         try:
-            f = open(self.HOSTNAME_FILE, 'w+')
+            with open(self.HOSTNAME_FILE, 'w+') as f:
-            try:
                 f.write("%s\n" % name)
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to update hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to update hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
     def set_current_hostname(self, name):
         cmd = [self.hostname_cmd, '-F', self.HOSTNAME_FILE]
@@ -440,42 +421,36 @@ class OpenRCStrategy(GenericStrategy):
     HOSTNAME_FILE = '/etc/conf.d/hostname'
 
     def get_permanent_hostname(self):
-        name = 'UNKNOWN'
+        if not os.path.isfile(self.HOSTNAME_FILE):
-        try:
+            return ''
+
         try:
-                f = open(self.HOSTNAME_FILE, 'r')
+            with open(self.HOSTNAME_FILE, 'r') as f:
                 for line in f:
                     line = line.strip()
                     if line.startswith('hostname='):
-                        name = line[10:].strip('"')
+                        return line[10:].strip('"')
-                        break
         except Exception as e:
-                self.module.fail_json(msg="failed to read hostname: %s" %
+            self.module.fail_json(
-                                          to_native(e), exception=traceback.format_exc())
+                msg="failed to read hostname: %s" % to_native(e),
-        finally:
+                exception=traceback.format_exc())
-            f.close()
-
-        return name
 
     def set_permanent_hostname(self, name):
         try:
-            try:
+            with open(self.HOSTNAME_FILE, 'r') as f:
-                f = open(self.HOSTNAME_FILE, 'r')
                 lines = [x.strip() for x in f]
 
                 for i, line in enumerate(lines):
                     if line.startswith('hostname='):
                         lines[i] = 'hostname="%s"' % name
                         break
-                f.close()
 
-                f = open(self.HOSTNAME_FILE, 'w')
+            with open(self.HOSTNAME_FILE, 'w') as f:
                 f.write('\n'.join(lines) + '\n')
         except Exception as e:
-                self.module.fail_json(msg="failed to update hostname: %s" %
+            self.module.fail_json(
-                                          to_native(e), exception=traceback.format_exc())
+                msg="failed to update hostname: %s" % to_native(e),
-        finally:
+                exception=traceback.format_exc())
-            f.close()
 
 
 class OpenBSDStrategy(GenericStrategy):
@@ -491,25 +466,21 @@ class OpenBSDStrategy(GenericStrategy):
             return ''
 
         try:
-            f = open(self.HOSTNAME_FILE)
+            with open(self.HOSTNAME_FILE) as f:
-            try:
                 return f.read().strip()
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to read hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to read hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
     def set_permanent_hostname(self, name):
         try:
-            f = open(self.HOSTNAME_FILE, 'w+')
+            with open(self.HOSTNAME_FILE, 'w+') as f:
-            try:
                 f.write("%s\n" % name)
-            finally:
-                f.close()
         except Exception as e:
-            self.module.fail_json(msg="failed to update hostname: %s" %
+            self.module.fail_json(
-                                      to_native(e), exception=traceback.format_exc())
+                msg="failed to update hostname: %s" % to_native(e),
+                exception=traceback.format_exc())
 
 
 class SolarisStrategy(GenericStrategy):
@@ -550,45 +521,39 @@ class FreeBSDStrategy(GenericStrategy):
     HOSTNAME_FILE = '/etc/rc.conf.d/hostname'
 
     def get_permanent_hostname(self):
-        name = 'UNKNOWN'
         if not os.path.isfile(self.HOSTNAME_FILE):
             return ''
 
         try:
-            try:
+            with open(self.HOSTNAME_FILE, 'r') as f:
-                f = open(self.HOSTNAME_FILE, 'r')
                 for line in f:
                     line = line.strip()
                     if line.startswith('hostname='):
-                        name = line[10:].strip('"')
+                        return line[10:].strip('"')
-                        break
         except Exception as e:
-                self.module.fail_json(msg="failed to read hostname: %s" %
+            self.module.fail_json(
-                                          to_native(e), exception=traceback.format_exc())
+                msg="failed to read hostname: %s" % to_native(e),
-        finally:
+                exception=traceback.format_exc())
-            f.close()
-
-        return name
 
     def set_permanent_hostname(self, name):
         try:
-            try:
+            if os.path.isfile(self.HOSTNAME_FILE):
-                f = open(self.HOSTNAME_FILE, 'r')
+                with open(self.HOSTNAME_FILE, 'r') as f:
                     lines = [x.strip() for x in f]
 
                     for i, line in enumerate(lines):
                         if line.startswith('hostname='):
                             lines[i] = 'hostname="%s"' % name
                             break
-                f.close()
+            else:
+                lines = ['hostname="%s"' % name]
 
-                f = open(self.HOSTNAME_FILE, 'w')
+            with open(self.HOSTNAME_FILE, 'w') as f:
                 f.write('\n'.join(lines) + '\n')
         except Exception as e:
-                self.module.fail_json(msg="failed to update hostname: %s" %
+            self.module.fail_json(
-                                          to_native(e), exception=traceback.format_exc())
+                msg="failed to update hostname: %s" % to_native(e),
-        finally:
+                exception=traceback.format_exc())
-            f.close()
 
 
 class DarwinStrategy(GenericStrategy):