canonical-ubuntu-qa team mailing list archive

[Dave Jones <mp+460043@xxxxxxxxxxxxxxxxxx>]

Review: Approve

This looks much better! I see ConfigParser.read is still in use (as opposed to ConfigParser.read_file), but there's no point blocking the merge for that as it really wouldn't matter to production anyway; if the config file were missing it'll fail with a missing key shortly after.

My main reason for preferring things to fail "as early as possible" is it aids debugging by not hiding (or more precisely misrepresenting the cause of) errors. Consider if something/someone accidentally renames the file autopkgtest_cloud.conf. The script fails with the aforementioned missing key; someone goes to investigate, doesn't notice the file is subtly different in name, and is then confused because the key is definitely present in the file. Versus using ConfigParser.read_file where it immediately fails with the file itself is missing (which would hopefully prod someone to look closely at the filename and notice - is not _).

Anyway, as I say, it's not worth holding up the merge for that -- thumbs up!
-- 
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/460043
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:sqlite-db-backup into autopkgtest-cloud:master.