Merge lp://qastaging/~sil2100/ubuntu-system-image/server-tags into lp://qastaging/ubuntu-system-image/server

Generally looks good. Some questions and comments inlined.

My high level concern is whether all the refactored code, and new code is adequately tested. Maybe the refactored code wasn't tested in the original branch, in which case, this might be a good opportunity to add *some* testing, even if it's incomplete.

But there's also bin/tag-image. Is this script tested anywhere? One thing you could think about is moving most of the logic to a module and then test that module, leaving bin/tag-image a thin wrapper around that module. Kind of like what you're doing with the refactored code above.

So I think I'd like to see a few more tests.

Thank you for the review Barry! So I made fixes as per your inline comments, although one I could not do. When removing the inner brackets from the len() expression, I get the following errors (on python3 here):

    size = len(x for x in version_detail_list if x.startswith("tag="))
TypeError: object of type 'generator' has no len()

So I left it as it is.

I fully agree that those scripts should be made testable and I already have a branch in the works for those. However, I did not want to introduce such big changes while adding new functionality as unit-testability didn't seem good enough reason to introduce regression risks by a rewrite, especially that this change is important for our current milestone. Even the new script I added follows the copy-image steps, so I thought it would be simply safer - for now - to leave it in the same style as the others. Unit tests do protect us from breaking functionality, but they depend on the fact that they cover all cases. When doing a rewrite I would feel much more comfortable having more time to test if the rewritten code works exactly the same as before.

So I would appreciate if we could shove the code in this way this week and then promise to finish up the testability re-write around next week, since the tagging functionality would be very welcome to have at the end of this week or the beginning of next one.

Thanks again!

So, just to state my general philosophy about tests, especially for established untested code. Tests are an investment and anything you can add increases the reliability of the code. If you've got an existing untested code base, you don't have to add tests for *everything*, but any new code added, or old code refactored is a great opportunity to increase that investment. For example, when refactoring, you can't be sure that you aren't breaking something if there are no tests. And the breakage is always more subtle than you can think about. So when refactoring, I like to add tests first, and that gives me more confidence that my refactoring will be solid.

Certainly any new code has to come with tests, otherwise it's just perpetuating the brokenness of the new code. As they say "untested code is broken code" :)

It's also okay to defer refactoring when you're trying to add a new feature. But maybe you can't because you can't add the feature without the refactoring. Tests really help to prove that you understand the semantics and how the refactoring will affect the functionality.

Well anyway, my $0.02!

Just one last inlined question.