Code review comment for lp:~bac/launchpad/lxc_cleanup

Revision history for this message
Gary Poster (gary) wrote :

Thank you Brad. Very nice. Your demo in the initial description did not also show that the process was killed, but you told me that this was accomplished as well, as I see in the code. Nice changes to how we write the sudoers file.

You could make the string raw, so that you didn't have to escape \"\"\" so many times. The only downside I see is that you'd have to do something clever about the first line, to make sure it has the shebang in it. Your call.

Maybe adding a comment about the fact that the unpleasant string parsing that you have to do in getPID mirrors what the approach done in lxc-list would be appropriate, to slightly reduce shock and surprise for someone reviewing this in the future.

The apparmor stuff (147-155 of diff) is supposed to be unnecessary now. I'd prefer to have that proved before we rip it out. Since you touched that code, I'm tempted to ask you to do something about it...but I'll forego, and add a separate card on the kanban board for the future.

You mentioned that you would send the unit tests for this code, so that we could have them available when we can divide things up better in lpsetup. I don't see that yet. Please make sure you do so. I'd hate to lose that work.

Thanks again!

review: Approve

« Back to merge proposal