Code review comment for lp:~matsubara/tarmac/bundle-lander

Gary Poster (gary) wrote :

Lock file: here are some things that would be nice, in decreasing order of importance in my mind.
 1 lock file is actually a pid file. pid is reported in error message. This gives SAs a chance to quickly see if it is dead or not.
 2 if lock file exists and has been around for fewer than N seconds, exit with status code 0. If lock file has been around for greater than or equal to N seconds, exit with a non-0 status code. N should be configurable. This gives SAs a chance to care about serious problems, and ignore typical problems.
 3 if lock file exists, error includes how long it has been around. That's easy enough to do without (just look at the file yourself), but a nice convenience.
 4 if lock file exists, with pid, and identified pid doesn't exist, delete the pid file, mention what you are doing, and proceed to run tarmac.

FWIW, lib/canonical/lazr/pidfile.py has some simple code to steal for #1.

I think I'll say that I want #1 unless you convince me otherwise. I'd be happy if you threw in #2 and #3, but I'm fine with ignoring it for now. I don't think you should pursue #4 right now.

Email: cool. A small note is that "(See the attached file for the complete log)" will be included in the merge proposal output, and that will be incorrect. Might be mildly confusing. Would be nice to omit it in the merge proposal, and only include it when sending the mail.

Nice tests. It test_run_failure, is it possible/reasonable to show that the merge proposal was changed, and that emails were sent? That seems like it would be nice.

« Back to merge proposal