Code review comment for lp://staging/~bjornt/charms/precise/landscape-client/container-relation-joined-test

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.