Code review comment for lp:~adam-collard/landscape-client/juju-info-at-registration

On 27 September 2013 15:32, Björn Tillenius <email address hidden> wrote:

> [1]
>
> 29 + self._juju_data = self._get_juju_data()
>
> I think it would be cleaner to collect the data when on "run", just
> like ec2 data is collected.
>

OK. Done!

>
> [2]
>
>
> 222 + self.assertMessages(self.mstore.get_pending_messages(),
> 223 + [{"type": "register",
> 224 + "computer_title":
> self.config.computer_title,
> 225 + "account_name": "account_name",
> 226 + "registration_password": None,
> 227 + "hostname": socket.getfqdn(),
> 228 + "vm-info": get_vm_info(),
> 229 + "tags": None,
> 230 + "juju-info": {
> 231 + "environment-uuid": "DEAD-BEEF",
> 232 + "api-addresses": ["10.0.3.1:17070"],
> 233 + "unit-name": "juju-unit-name"}
> 234 + }])
>
> The indentation looks off here. Also, please use valid unit names,
> like service/0.
>

Emacs wanted to indent it like that - have shifted things around a bit.

Done!

>
>
> [3]
> 264 - "tags": None}])
> 265 + "tags": None,
> 266 + "juju-info": {
> 267 + "environment-uuid": "DEAD-BEEF",
> 268 + "api-addresses": ["10.0.3.1:17070
> "],
> 269 + "unit-name": "juju-unit-name"}}])
>
> Indentation is off here as well.
>

This became irrelevant because of [6]

>
> [4]
> 365 + def test_get_juju_info_sample_data(self):
> 366 + """L{get_juju_info} works with sample data."""
>
> What does "works with sample data" mean? It's implied that our
> code works. Better to explain what the code is expected to do instead.
>

I've tried re-phrasing it. It really is just "does it work with realistic
data"

>
> [5]
> 384 + self.assertTrue(
> 385 + "Error attempting to read JSON" in
> self.logfile.getvalue())
>
> Please use assertIn instead, which gives a better error message on
> failures.
>

Argh. I thought we didn't have it because client has to work on 2.6, but
mocker implements it. Grr, I had it as assertIn and the other test using
assertIs then carefully undid them. Thanks :)

>
>
> [6]
>
> 435 + "juju-info": KeyDict(juju_data)},
> 436 + optional=["tags", "vm-info", "public_ipv4", "local_ipv4",
> 437 + "juju-info"])
>
> I think we shouldn't include juju-info in the register-cloud-vm message.
> By default Juju instances won't use that the register-cloud-vm message,
> and the plan is to remove it altogether.
>

OK, I wasn't sure what this was so erred on the side of caution and
included it. Removed.