Merge lp://staging/~bjornt/charms/precise/landscape-client/container-relation-joined-test into lp://staging/charms/landscape-client

Looks like an improvement to me, +1!

[0]

+ def get_environment_config(self):
+ """Get the shell environment variables."""
+ return self.environment

It feels a bit verbose to add a getter method for a public attribute. I'd rather make the attribute private or drop the method.

[1]

+class TestingJujuBroker(JujuBroker):

I'd probably rename this to FakeJujuBroker, for consistency with the naming that we generally use in the client and server code (and it matches the widely accepted terminology http://xunitpatterns.com/Fake%20Object.html)

TestingLandscapeBroker too.

[2]

+ self.landscape_broker = TestingLandscapeBroker(data_path=data_path)
+ self.landscape_broker._config.config = os.path.join(
+ data_path, "landscape.conf")
+ self.landscape_broker._config.write()

This boilerplate isn't great, and it also accesses the private _config attribute. I think one problem is that TestingLandscapeBroker tries to abstract away the interaction with the client, but it actually leaks it by returning the config object with get_client_config, which forces tests to manage such config object. I don't have an easy solution, so I think this good enough as a first step in a better direction.

[3]

+ with open(juju_info_path, "r") as juju_file:
+ juju_info = json.loads(juju_file.read())

You could use landscape.lib.fs.read_file here:

            juju_info = json.loads(read_file(juju_info_path))

On Fri, Oct 18, 2013 at 09:48:33AM -0000, Free Ekanayaka wrote:
> Review: Approve
>
> Looks like an improvement to me, +1!
>
> [0]
>
> + def get_environment_config(self):
> + """Get the shell environment variables."""
> + return self.environment
>
> It feels a bit verbose to add a getter method for a public attribute.
> I'd rather make the attribute private or drop the method.

Agreed, I removed the method.

> [1]
>
> +class TestingJujuBroker(JujuBroker):
>
> I'd probably rename this to FakeJujuBroker, for consistency with the naming that we generally use in the client and server code (and it matches the widely accepted terminology http://xunitpatterns.com/Fake%20Object.html)
>
> TestingLandscapeBroker too.

Fair enough, renamed.

> [2]
>
> + self.landscape_broker = TestingLandscapeBroker(data_path=data_path)
> + self.landscape_broker._config.config = os.path.join(
> + data_path, "landscape.conf")
> + self.landscape_broker._config.write()
>
> This boilerplate isn't great, and it also accesses the private _config
> attribute. I think one problem is that TestingLandscapeBroker tries to
> abstract away the interaction with the client, but it actually leaks it
> by returning the config object with get_client_config, which forces
> tests to manage such config object. I don't have an easy solution, so I
> think this good enough as a first step in a better direction.

Yeah, I wasn't proud of that :) One problem was that we had a
get_client_config() that reloaded the config. I don't know why the
config is reloaded, but I changed is so that LandscapeBroker now creates
a Configuration object and exposes it as the .config attribute. I
changed the callsites to call landscape_broker.config.reload().

I also removed the data_path argument to the constructor, since it
didn't make sense to pass in data_path and then set .config.config
outside the constructor.

> [3]
>
> + with open(juju_info_path, "r") as juju_file:
> + juju_info = json.loads(juju_file.read())
>
> You could use landscape.lib.fs.read_file here:
>
> juju_info = json.loads(read_file(juju_info_path))

Done.

Looks great! This is really nice. +1

[1]
+class JujuBroker:
and
+class LandscapeBroker:

Nitpick: maybe use new-style classes by subclassing object?

[2]
+ If the client has already been configured, not registration attempt

Small typo s/not/no/ (was not your code).