Merge lp:~ack/landscape-client/package-reporter-permissions into lp:~landscape/landscape-client/trunk

[Alberto Donato]

This fixes bug #804008, running package reporter as user 'landscape' also APT sources changes.

Also, it introduces a general-purpose process launcher using Twisted, refactoring some existing code to make it reusable.

[Thomas Herve (therve)]

[1]
- def run_process(self, command, args):
+ def _run_process(self, command, args, env={}, path=None, uid=None, gid=None):

I don't see the point of having env and path here, as you never pass them. But you always pass uid and gid, so they shouldn't be optional.

+1!

review: Approve

[Alberto Donato (ack)]

> [1]
> - def run_process(self, command, args):
> + def _run_process(self, command, args, env={}, path=None, uid=None,
> gid=None):
>
> I don't see the point of having env and path here, as you never pass them. But
> you always pass uid and gid, so they shouldn't be optional.

Uid and gid are not passed in the call to apt-key since it is run as root.
What about dropping env and path and keep uid/gid optional?

>
> +1!

[Thomas Herve (therve)]

Ah right, sounds good.

[Alberto Donato (ack)]

+1!

[1]
In test_management.py some test comments (and names) refer to C{newest_login_time} which is actually last_login_time.

review: Approve

[Alberto Donato (ack)]

Commented and approved the wrong MP.

review: Abstain

[lp:~ack/landscape-client/package-reporter-permissions]

343. By Alberto Donato on 2011-07-07

Merge trunk

[Free Ekanayaka (free.ekanayaka)]

Nice branch, +1!

[1]

+def spawn_process(executable, args=(), env={}, path=None, uid=None, gid=None,
+ wait_pipes=True):

Please add a @param for describing what wait_pipes does.

[2]

+ def tearDown(self):
+ os.unlink(self.command)

No need for this, everything created with self.makeFile will be automatically removed.

[3]
        """
        The process returns the expected standard error.
        """

        self._write_command("#!/bin/sh\necho -n $@ >&2")

There's an extra blank between the docstring and the first line of the test.

[4]

+ def _write_command(self, command):

I'd suggest to drop this and use landscape.lib.fs.create_file instead (which maybe should be renamed write_file):

from landscape.lib.fs import create_file

    def test_spawn_process_return_value(self):
        create_file(self.command, "#!/bin/sh\nexit 2")

        def callback((out, err, code)):

This is because the less ad-hoc methods the test has the more readable it is.

review: Approve

[lp:~ack/landscape-client/package-reporter-permissions]

344. By Alberto Donato on 2011-07-07

Addressed Free's comments, fixed tests.