Merge lp:~adam-collard/landscape-client/config-file-sysinfo into lp:~landscape/landscape-client/trunk

[Adam Collard]

Only complain about missing default configuration files if they are present but unreadable.

Only complain about missing default configuration files if they are present but unreadable.

To test:

rm /etc/landscape/client.conf (or move it out of the way if it's special)

landscape-sysinfo should not error out.

[Björn Tillenius (bjornt)]

[1]

This code:

51 + if os.path.isfile(self.config) and os.access(self.config, os.R_OK):
52 + self.load_configuration_file(self.config)
53 + elif not accept_nonexistent_config:
54 + sys.exit("error: config file %s can't be read" % self.config)

looks a lot like this code:

77 + if os.path.isfile(config_filename):
78 + if not os.access(config_filename, os.R_OK):
79 + errors.append("error: config file %s can't be read" %
80 + config_filename)
81 + else:
82 + self.load_configuration_file(config_filename)
83 + break

I had trouble seeing the difference at first. Only after re-reading the code several
time I spotted what was different.

I'd suggest being a bit more explicit and write the whole think like this:

        if (self.config and not os.path.isfile(self.config)
                and not accept_nonexistent_config):
            sys.exit("error: config file %s doesn't exist" % self.config)

        if self.config:
            config_filenames = [self.config]
        else:
            config_filenames = self.default_config_filenames

        errors = []
        for config_filename in config_filenames:
            if os.path.isfile(config_filename):
                if not os.access(config_filename, os.R_OK):
                    errors.append("error: config file %s can't be read" %
                                  config_filename)
                else:
                    self.load_configuration_file(config_filename)
                    break
        else:
            if errors and not accept_nonexistent_config:
                sys.exit("\n".join(errors))

I.e., first check whether self.config exists, and then do the general checking
that was already in place. What do you think?

review: Needs Information

[Geoff Teale (tealeg)]

+1 Approve

With regards to Bjorn's suggestion above - it seem quite good, but I'm slightly concerned that we wouldn't append an error if os.path.isfile returns false for a filename in self.default_config_filenames. That should be done if you choose to adopt Bjorn's code.

review: Approve

[Adam Collard (adam-collard)]

> +1 Approve
>
> With regards to Bjorn's suggestion above - it seem quite good, but I'm
> slightly concerned that we wouldn't append an error if os.path.isfile returns
> false for a filename in self.default_config_filenames. That should be done if
> you choose to adopt Bjorn's code.

Hehe, that would take me back to where I started.

We don't want errors if /etc/landscape/client.conf is missing, only if it's present, but unreadable

[Adam Collard (adam-collard)]

> [1]
>
> This code:
>
> 51 + if os.path.isfile(self.config) and os.access(self.config,
> os.R_OK):
> 52 + self.load_configuration_file(self.config)
> 53 + elif not accept_nonexistent_config:
> 54 + sys.exit("error: config file %s can't be read" %
> self.config)
>
>
> looks a lot like this code:
>
> 77 + if os.path.isfile(config_filename):
> 78 + if not os.access(config_filename, os.R_OK):
> 79 + errors.append("error: config file %s can't be
> read" %
> 80 + config_filename)
> 81 + else:
> 82 + self.load_configuration_file(config_filename)
> 83 + break
>
> I had trouble seeing the difference at first. Only after re-reading the code
> several
> time I spotted what was different.

Yeah sorry about that. I'll add some comments to ease readability too.

>
> I'd suggest being a bit more explicit and write the whole think like this:
>
> if (self.config and not os.path.isfile(self.config)
> and not accept_nonexistent_config):
> sys.exit("error: config file %s doesn't exist" % self.config)
>
> if self.config:
> config_filenames = [self.config]
> else:
> config_filenames = self.default_config_filenames
>
> errors = []
> for config_filename in config_filenames:
> if os.path.isfile(config_filename):
> if not os.access(config_filename, os.R_OK):
> errors.append("error: config file %s can't be read" %
> config_filename)
> else:
> self.load_configuration_file(config_filename)
> break
> else:
> if errors and not accept_nonexistent_config:
> sys.exit("\n".join(errors))
>
>
> I.e., first check whether self.config exists, and then do the general checking
> that was already in place. What do you think?

I'm -0.5 on the suggestion - I don't like checking for it being a readable file twice. Once we've got past the first if we've already checked that self.config is a readable file.

I think how I have it now is more explicit, even if more verbose. There are two different behaviours for a specified configuration file vs. default configuration files, and those behaviours are in two separate blocks.

An alternative would be to try...except here and have different ways of handling the exception

[Björn Tillenius (bjornt)]

I still prefer my version, since it doesn't repeat the code so much.

But if you still want to go with your version, please add comments
for the place where you check whether the file exists and the
permission, to make it clear that when you specify the config
file on the command line, the file has to be there, and when
using the default files, only the permission is interesting.

[2]

Sorry for not spotting, this before, but this branch actually
changes the behavior, so that the name accept_nonexistent_config
isn't accurate anymore. If no default config files are found,
it doesn't matter what value accept_nonexisting_config has,
no error will be raised.

Do you know what this variable is used for?

review: Needs Information

[Björn Tillenius (bjornt)]

Thanks, removing the accept_nonexisting_config makes it look a lot
cleaner, +1!

review: Approve

[🤖 Landscape Builder (landscape-builder)]

The attempt to merge lp:~adam-collard/landscape-client/config-file-sysinfo into lp:landscape-client failed. Below is the output from the failed tests.

python setup.py build_ext -i
running build_ext
landscape.broker.tests.test_amp
  RemoteBrokerTest
    test_call_if_accepted ... [ERROR]
    test_call_if_accepted_with_not_accepted ... [ERROR]
    test_call_on_events ... [ERROR]
    test_exit ... [ERROR]
    test_fire_event ... [ERROR]
    test_get_accepted_message_types ... [ERROR]
    test_get_server_uuid ... [ERROR]
    test_is_message_pending ... [ERROR]
    test_listen_events ... [ERROR]
    test_method_call_error ... [ERROR]
    test_ping ... [ERROR]
    test_register ... [ERROR]
    test_register_client ... [ERROR]
    test_register_client_accepted_message_type ... [ERROR]
    test_reload_configuration ... [ERROR]
    test_send_message ... [ERROR]
    test_send_message_with_urgent ... [ERROR]
    test_stop_clients ... [ERROR]
  RemoteClientTest
    test_exit ... [ERROR]
    test_fire_event ... [ERROR]
    test_message ... [ERROR]
    test_method_call_error ... [ERROR]
    test_ping ... [ERROR]
landscape.broker.tests.test_client
  BrokerClientTest
    test_add ... [ERROR]
    test_dispatch_message ... [ERROR]
    test_dispatch_message_with_exception ... [ERROR]
    test_dispatch_message_with_no_handler ... [ERROR]
    test_exchange ... [ERROR]
    test_exchange_logs_errors_and_continues ... [ERROR]
    test_exchange_on_plugin_without_exchange_method ... [ERROR]
    test_exit ... [ERROR]
    test_fire_event ... [ERROR]
    test_fire_event_with_acceptance_changed ... [ERROR]
    test_fire_event_with_arguments ... [ERROR]
    test_fire_event_with_mixed_results ... [ERROR]
    test_get_named_plugin ... [ERRO...

[lp:~adam-collard/landscape-client/config-file-sysinfo]

763. By Adam Collard on 2014-03-25

Remove unused variable (oops)

=== modified file 'landscape/__init__.py'
--- landscape/__init__.py	2013-12-17 09:44:42 +0000
+++ landscape/__init__.py	2014-03-25 09:06:18 +0000
@@ -1,5 +1,5 @@
 DEBIAN_REVISION = ""
-UPSTREAM_VERSION = "13.10"
+UPSTREAM_VERSION = "14.01"
 VERSION = "%s%s" % (UPSTREAM_VERSION, DEBIAN_REVISION)
 
 # The "server-api" field of outgoing messages will be set to this value, and

=== modified file 'landscape/broker/config.py'
--- landscape/broker/config.py	2013-11-25 09:55:59 +0000
+++ landscape/broker/config.py	2014-03-25 09:06:18 +0000
@@ -79,7 +79,7 @@
         """Get the path to the message store."""
         return os.path.join(self.data_path, "messages")
 
-    def load(self, args, accept_nonexistent_config=False):
+    def load(self, args):
         """
         Load options from command line arguments and a config file.
 
@@ -87,8 +87,7 @@
         C{http_proxy} and C{https_proxy} environment variables based on
         that config data.
         """
-        super(BrokerConfiguration, self).load(
-            args, accept_nonexistent_config=accept_nonexistent_config)
+        super(BrokerConfiguration, self).load(args)
         if self.http_proxy:
             os.environ["http_proxy"] = self.http_proxy
         elif self._original_http_proxy:

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2014-02-25 18:06:48 +0000
+++ landscape/configuration.py	2014-03-25 09:06:18 +0000
@@ -411,7 +411,7 @@
             self.prompt("tags", "Tags", False)
             if self._get_invalid_tags(self.config.tags):
                 self.show_help("Tag names may only contain alphanumeric "
-                              "characters.")
+                               "characters.")
                 self.config.tags = None  # Reset for the next prompt
             else:
                 break

=== modified file 'landscape/deployment.py'
--- landscape/deployment.py	2013-10-22 21:34:31 +0000
+++ landscape/deployment.py	2014-03-25 09:06:18 +0000
@@ -63,7 +63,7 @@
     unsaved_options = ()
     default_config_filenames = ["/etc/landscape/client.conf"]
     if (os.path.dirname(os.path.abspath(sys.argv[0]))
-        == os.path.abspath("scripts")):
+          == os.path.abspath("scripts")):
         default_config_filenames.insert(0, "landscape-client.conf")
     default_config_filenames = tuple(default_config_filenames)
     config_section = "client"
@@ -148,7 +148,7 @@
         """
         self.load(self._command_line_args)
 
-    def load(self, args, accept_nonexistent_config=False):
+    def load(self, args):
         """
         Load configuration data from command line arguments and a config file.
 
@@ -156,28 +156,26 @@
         """
         self.load_command_line(args)
 
+        if self.config and not os.path.isfile(self.config):
+            sys.exit("error: config file %s doesn't exist" % self.config)
+
         if self.config:
             config_filenames = [self.config]
         else:
             config_filenames = self.default_config_filenames
-        # Parse configuration file, if found.
+
+        errors = []
         for config_filename in config_filenames:
-            if (os.path.isfile(config_filename)
-                and os.access(config_filename, os.R_OK)):
-
-                self.load_configuration_file(config_filename)
-                break
-
+            if os.path.isfile(config_filename):
+                if not os.access(config_filename, os.R_OK):
+                    errors.append("error: config file %s can't be read" %
+                                  config_filename)
+                else:
+                    self.load_configuration_file(config_filename)
+                    break
         else:
-            if not accept_nonexistent_config:
-                if len(config_filenames) == 1:
-                    message = (
-                        "error: config file %s can't be read" %
-                        config_filenames[0])
-                else:
-                    message = "error: no config file could be read"
-                sys.exit(message)
-
+            if errors:
+                sys.exit("\n".join(errors))
         self._load_external_options()
 
         # Check that all needed options were given.
@@ -402,12 +400,11 @@
 
         return parser
 
-    def load(self, args, accept_nonexistent_config=False):
+    def load(self, args):
         """
         Load configuration data from command line arguments and a config file.
         """
-        super(Configuration, self).load(
-            args, accept_nonexistent_config=accept_nonexistent_config)
+        super(Configuration, self).load(args)
 
         if not isinstance(self.server_autodiscover, bool):
             autodiscover = str(self.server_autodiscover).lower()

=== modified file 'landscape/tests/test_deployment.py'
--- landscape/tests/test_deployment.py	2013-10-24 01:55:54 +0000
+++ landscape/tests/test_deployment.py	2014-03-25 09:06:18 +0000
@@ -196,8 +196,7 @@
         self.config.log_level = "warning"
         self.config.write()
         data = open(self.config_filename).read()
-        self.assertConfigEqual(data,
-            "[client]\nlog_level = warning\n")
+        self.assertConfigEqual(data, "[client]\nlog_level = warning\n")
 
     def test_write_empty_list_values_instead_of_double_quotes(self):
         """
@@ -303,10 +302,8 @@
         self.assertConfigEqual(data, "[client]\nlog_level = error\n")
 
     def test_write_to_given_config_file(self):
-        filename = self.makeFile()
-        self.config.load(
-            ["--log-level", "warning", "--config", filename],
-            accept_nonexistent_config=True)
+        filename = self.makeFile(content="")
+        self.config.load(["--log-level", "warning", "--config", filename])
         self.config.log_level = "error"
         self.config.write()
         data = open(filename).read()
@@ -383,7 +380,8 @@
         """
         filename = self.makeFile("[client]\nhello = world1\n")
         self.config.load(["--config", filename])
-        open(filename, "w").write("[client]\nhello = world2\n")
+        with open(filename, "w") as fh:
+            fh.write("[client]\nhello = world2\n")
         self.config.reload()
         self.assertEqual(self.config.hello, "world2")
 
@@ -408,7 +406,7 @@
         error = self.assertRaises(
             SystemExit, self.config.load, ["--config", filename])
         self.assertEqual(
-            "error: config file %s can't be read" % filename, str(error))
+            "error: config file %s doesn't exist" % filename, str(error))
 
     def test_load_cannot_read_default(self):
         """
@@ -424,13 +422,11 @@
 
     def test_load_not_found_default(self):
         """
-        C{config.load} exits the process if the default config file is not
-        found.
+        C{config.load} doesn't exit the process if the default config file
+        is not found.
         """
         [default] = self.config.default_config_filenames[:] = ["/not/here"]
-        error = self.assertRaises(SystemExit, self.config.load, [])
-        self.assertEqual(
-            "error: config file %s can't be read" % default, str(error))
+        self.config.load([])
 
     def test_load_cannot_read_many_defaults(self):
         """
@@ -444,7 +440,8 @@
         self.config.default_config_filenames[:] = [default1, default2]
 
         error = self.assertRaises(SystemExit, self.config.load, [])
-        self.assertEqual("error: no config file could be read", str(error))
+        self.assertEqual(
+            "error: config file %s can't be read" % default1, str(error))
 
     def test_data_directory_option(self):
         """Ensure options.data_path option can be read by parse_args."""