Merge lp:~free.ekanayaka/landscape-client/drop-config-bus into lp:~landscape/landscape-client/trunk

[Free Ekanayaka]

This branch drops the Configuration.bus option, which was used to select the DBus bus to connect to.

[Thomas Herve (therve)]

[1]
+ if not (os.getuid() == 0 or
+ pwd.getpwnam("landscape").pw_uid == os.getuid()):
         sys.exit("When using the system bus, landscape-client must be run as "
                  "root.")

It seems that we can remove this check completely?

review: Needs Fixing

[Jamu Kakar (jkakar)]

[1]

+ info("Watchdog watching for daemons")

Please add a period to the end of this sentence.

[2]

+ if not (os.getuid() == 0 or
+ pwd.getpwnam("landscape").pw_uid == os.getuid()):
         sys.exit("When using the system bus, landscape-client must be run as "
                  "root.")

If the 'landscape' user doesn't exist a KeyError will be raised by
pwd.getpwnam. The user will get a message like:

KeyError: 'getpwnam(): name not found: landscape'

I recommend making this a bit more friendly by displaying a more
easily understandable message if the 'landscape' user isn't present:

    try:
        landscape_uid = pwd.getpwnam("landscape").pw_uid
    except KeyError:
        sys.exit("The 'landscape' user doesn't exist!")
    else:
        if os.getuid() not in (0, landscape_uid):
             sys.exit("landscape-client can only be run as 'root' "
                      "or 'landscape'.")

[3]

The docstring for the 'run' function needs to be updated.

+1 with these issues fixed.

review: Approve

[lp:~free.ekanayaka/landscape-client/drop-config-bus]

255. By Free Ekanayaka on 2010-06-10

Fix Thomas' [1] and Jamu's [1] to [3]

[Free Ekanayaka (free.ekanayaka)]

Thanks Thomas and Jamu, I think it's all fixed!

[Thomas Herve (therve)]

+1!

review: Approve

=== modified file 'landscape/deployment.py'
--- landscape/deployment.py	2010-05-07 10:51:41 +0000
+++ landscape/deployment.py	2010-06-10 08:55:43 +0000
@@ -153,9 +153,6 @@
                          "or the '%s' directive in the config file."
                          % (option.replace('_', '-'), option))
 
-        if self.bus not in ("session", "system"):
-            sys.exit("error: bus must be one of 'session' or 'system'")
-
     def _load_external_options(self):
         """Hook for loading options from elsewhere (e.g. for --import)."""
 
@@ -223,16 +220,12 @@
         @return: An L{OptionParser} preset with options that all
             landscape-related programs accept. These include
               - C{config} (C{None})
-              - C{bus} (C{system})
         """
         parser = OptionParser(version=VERSION)
         parser.add_option("-c", "--config", metavar="FILE",
                           help="Use config from this file (any command line "
                                "options override settings from the file) "
                                "(default: '/etc/landscape/client.conf').")
-        parser.add_option("--bus", default="system",
-                          help="Which DBUS bus to use. One of 'session' "
-                               "or 'system' (default: 'system').")
         return parser
 
     def get_config_filename(self):

=== modified file 'landscape/tests/test_deployment.py'
--- landscape/tests/test_deployment.py	2010-05-03 13:08:02 +0000
+++ landscape/tests/test_deployment.py	2010-06-10 08:55:43 +0000
@@ -11,6 +11,7 @@
 class BabbleConfiguration(Configuration):
     config_section = "babble"
     default_config_filenames = []
+
     def make_parser(self):
         parser = super(BabbleConfiguration, self).make_parser()
         parser.add_option("--whatever", metavar="STUFF")
@@ -25,9 +26,11 @@
 
     def reset_config(self, configuration_class=None):
         if not configuration_class:
+
             class MyConfiguration(Configuration):
                 default_config_filenames = []
             configuration_class = MyConfiguration
+
         self.config_class = configuration_class
         self.config = configuration_class()
         self.parser = self.config.make_parser()
@@ -51,18 +54,23 @@
         self.assertEquals(self.config.log_level, "command line")
 
     def test_command_line_option_without_default(self):
+
         class MyConfiguration(Configuration):
+
             def make_parser(self):
                 parser = OptionParser()
                 # Keep the dash in the option name to ensure it works.
                 parser.add_option("--foo-bar")
                 return parser
+
         self.assertEquals(MyConfiguration().foo_bar, None)
 
     def test_command_line_with_required_options(self):
+
         class MyConfiguration(Configuration):
             required_options = ("foo_bar",)
             config = None
+
             def make_parser(self):
                 parser = super(MyConfiguration, self).make_parser()
                 # Keep the dash in the option name to ensure it works.
@@ -81,14 +89,17 @@
         self.assertEquals(self.config.foo_bar, "ooga")
 
     def test_command_line_with_unsaved_options(self):
+
         class MyConfiguration(Configuration):
             unsaved_options = ("foo_bar",)
             config = None
+
             def make_parser(self):
                 parser = super(MyConfiguration, self).make_parser()
                 # Keep the dash in the option name to ensure it works.
                 parser.add_option("--foo-bar", metavar="NAME")
                 return parser
+
         self.reset_config(configuration_class=MyConfiguration)
         self.write_config_file()
 
@@ -112,9 +123,11 @@
 
     def test_no_section_available(self):
         config_filename = self.makeFile("")
+
         class MyConfiguration(Configuration):
             config_section = "nonexistent"
             default_config_filenames = (config_filename,)
+
         self.reset_config(configuration_class=MyConfiguration)
         self.config.load([])
 
@@ -220,16 +233,6 @@
         data = open(filename).read()
         self.assertEquals(data.strip(), "[client]\nlog_level = error")
 
-    def test_bus_option(self):
-        """The bus option must be specified as 'system' or 'session'."""
-        self.assertRaises(SystemExit,
-                          self.config.load,
-                          ["--bus", "foobar"])
-        self.config.load(["--bus", "session"])
-        self.assertEquals(self.config.bus, "session")
-        self.config.load(["--bus", "system"])
-        self.assertEquals(self.config.bus, "system")
-
     def test_config_option(self):
         opts = self.parser.parse_args(["--config", "hello.cfg"])[0]
         self.assertEquals(opts.config, "hello.cfg")
@@ -240,22 +243,28 @@
         self.assertEquals(self.config.hello, "world")
 
     def test_load_typed_option_from_file(self):
+
         class MyConfiguration(self.config_class):
+
             def make_parser(self):
                 parser = super(MyConfiguration, self).make_parser()
                 parser.add_option("--year", default=1, type="int")
                 return parser
+
         filename = self.makeFile("[client]\nyear = 2008\n")
         config = MyConfiguration()
         config.load(["--config", filename])
         self.assertEquals(config.year, 2008)
 
     def test_load_typed_option_from_command_line(self):
+
         class MyConfiguration(self.config_class):
+
             def make_parser(self):
                 parser = super(MyConfiguration, self).make_parser()
                 parser.add_option("--year", default=1, type="int")
                 return parser
+
         config = MyConfiguration()
         config.load(["--year", "2008"])
         self.assertEquals(config.year, 2008)
@@ -346,6 +355,7 @@
 class GetVersionedPersistTest(LandscapeTest):
 
     def test_upgrade_service(self):
+
         class FakeService(object):
             persist_filename = self.makePersistFile(content="")
             service_name = "monitor"

=== modified file 'landscape/tests/test_watchdog.py'
--- landscape/tests/test_watchdog.py	2010-05-03 13:08:02 +0000
+++ landscape/tests/test_watchdog.py	2010-06-10 08:55:43 +0000
@@ -963,8 +963,7 @@
         self.configuration = WatchDogConfiguration()
         self.data_path = self.makeDir()
         self.log_dir = self.makeDir()
-        self.configuration.load(["--bus", "system",
-                                 "--data-path", self.data_path,
+        self.configuration.load(["--data-path", self.data_path,
                                  "--log-dir", self.log_dir])
 
     def test_daemonize(self):
@@ -1316,8 +1315,9 @@
         getpwnam("landscape").pw_uid
         self.mocker.result(1001)
         self.mocker.replay()
-        sys_exit = self.assertRaises(SystemExit, run, ["--bus", "system"])
-        self.assertIn("landscape-client must be run as root", str(sys_exit))
+        sys_exit = self.assertRaises(SystemExit, run)
+        self.assertIn("landscape-client can only be run"
+                      " as 'root' or 'landscape'.", str(sys_exit))
 
     def test_landscape_user(self):
         """
@@ -1328,11 +1328,27 @@
         self.mocker.result(os.getuid())
         self.mocker.replay()
         reactor = FakeReactor()
-        run(["--bus", "system", "--log-dir", self.makeFile()],
-            reactor=reactor)
+        run(["--log-dir", self.makeFile()], reactor=reactor)
         self.assertTrue(reactor.running)
 
+    def test_non_root(self):
+        """
+        The watchdog should print an error message and exit if the
+        'landscape' user doesn't exist.
+        """
+        getpwnam = self.mocker.replace("pwd.getpwnam")
+        getpwnam("landscape")
+        self.mocker.throw(KeyError())
+        self.mocker.replay()
+        sys_exit = self.assertRaises(SystemExit, run)
+        self.assertIn("The 'landscape' user doesn't exist!", str(sys_exit))
+
     def test_clean_environment(self):
+        getpwnam = self.mocker.replace("pwd.getpwnam")
+        getpwnam("landscape").pw_uid
+        self.mocker.result(os.getuid())
+        self.mocker.replay()
+
         os.environ["DEBIAN_YO"] = "yo"
         os.environ["DEBCONF_YO"] = "yo"
         os.environ["LANDSCAPE_ATTACHMENTS"] = "some attachments"
@@ -1340,8 +1356,7 @@
         os.environ["UNRELATED"] = "unrelated"
 
         reactor = FakeReactor()
-        run(["--bus", "session", "--log-dir", self.makeFile()],
-            reactor=reactor)
+        run(["--log-dir", self.makeFile()], reactor=reactor)
         self.assertNotIn("DEBIAN_YO", os.environ)
         self.assertNotIn("DEBCONF_YO", os.environ)
         self.assertNotIn("LANDSCAPE_ATTACHMENTS", os.environ)

=== modified file 'landscape/watchdog.py'
--- landscape/watchdog.py	2010-05-03 13:08:02 +0000
+++ landscape/watchdog.py	2010-06-10 08:55:43 +0000
@@ -496,7 +496,7 @@
                 reactor.crash() # so stopService isn't called.
                 return
             self._daemonize()
-            info("Watchdog watching for daemons on %r bus." % self._config.bus)
+            info("Watchdog watching for daemons.")
             return self.watchdog.start()
 
         def die(failure):
@@ -582,19 +582,20 @@
     @param reactor: The reactor to use.  If none is specified, the global
         reactor is used.
     @raise SystemExit: if command line arguments are bad, or when landscape-
-        client is running as non-root and the configuration indicates use of
-        the system bus.
+        client is not running as 'root' or 'landscape'.
     """
     clean_environment()
 
     config = WatchDogConfiguration()
     config.load(args)
 
-    if (config.bus == "system"
-        and not (os.getuid() == 0
-                 or pwd.getpwnam("landscape").pw_uid == os.getuid())):
-        sys.exit("When using the system bus, landscape-client must be run as "
-                 "root.")
+    try:
+        landscape_uid = pwd.getpwnam("landscape").pw_uid
+    except KeyError:
+        sys.exit("The 'landscape' user doesn't exist!")
+
+    if os.getuid() not in (0, landscape_uid):
+        sys.exit("landscape-client can only be run as 'root' or 'landscape'.")
 
     init_logging(config, "watchdog")