UTAH

Code review comment for lp:~doanac/utah/rsyslog-v4

rsyslog-v4
Merge into dev

Revision history for this message

Andy Doan (doanac) wrote on 2013-03-05:

and my comments to his comments:

On 03/05/2013 10:41 AM, Max Brustkern wrote:
> So, it looks like you've added some generic exception logging to
> run_test_vm.py, but not any of the other test scripts. I forgot that
> script even exists; it used to be the script to run the vm-tools
> integration, but that integration has been deprecated for a while. I
> think if we're changing one run script, we should probably be
> changing them all or changing run.py. I think we should also
> consider whether changing them at all makes sense, given the
> likelihood that we'll be removing most of them soon in a separate
> merge.

I fixed the one because it happened to be swallowing a syntax error I
discovered while testing my code. I don't mind updating the others - it
might save someone else 20 minutes in the future.

> On lines 270 and 401, I'd consider using a generic hostname, like
> ubuntu. I'd also consider picking a weird specific date, like the
> beginning of the unix epoch, or the date and time I was born, or when
> ubuntu was first released, but I'm weird like that.

sure.

> On 468 and 532 we're still checking for SSH in a retry loop with a
> long timeout, even though we know the machine is installed and
> booted. I think this is reasonable behavior, at least for now, but I
> think a comment in there will help us remember that we may want to
> revise that at some point. Similarly, notes on lines 938 and 960
> might make sense.

I suppose we should just update our default config now, but I wasn't
sure if this would play nice with reboot support changes?

> Since you've taken out the removal of the serial device on line 945,
> have you verified that the log correctly appends after a reboot and
> doesn't just overwrite the old one? I think I've had problems with
> that before.

It does not append to the file. However, the rsyslog.py code creates a
-install.log file that has all of that.

> I haven't actually reviewed the new rsyslog.py yet, but those are my
> thoughts on everything else.

Cool - i'll make some updates ASAP

and my comments to his comments:

On 03/05/2013 10:41 AM, Max Brustkern wrote:
> So, it looks like you've added some generic exception logging to
> run_test_vm.py, but not any of the other test scripts.  I forgot that
> script even exists; it used to be the script to run the vm-tools
> integration, but that integration has been deprecated for a while.  I
> think if we're changing one run script, we should probably be
> changing them all or changing run.py.  I think we should also
> consider whether changing them at all makes sense, given the
> likelihood that we'll be removing most of them soon in a separate
> merge.

I fixed the one because it happened to be swallowing a syntax error I
discovered while testing my code. I don't mind updating the others - it
might save someone else 20 minutes in the future.

> On lines 270 and 401, I'd consider using a generic hostname, like
> ubuntu.  I'd also consider picking a weird specific date, like the
> beginning of the unix epoch, or the date and time I was born, or when
> ubuntu was first released, but I'm weird like that.

sure.

> On 468 and 532 we're still checking for SSH in a retry loop with a
> long timeout, even though we know the machine is installed and
> booted.  I think this is reasonable behavior, at least for now, but I
> think a comment in there will help us remember that we may want to
> revise that at some point.  Similarly, notes on lines 938 and 960
> might make sense.

I suppose we should just update our default config now, but I wasn't
sure if this would play nice with reboot support changes?

It does not append to the file. However, the rsyslog.py code creates a
-install.log file that has all of that.

> I haven't actually reviewed the new rsyslog.py yet, but those are my
> thoughts on everything else.

Cool - i'll make some updates ASAP

« Back to merge proposal