Tarmac

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

bundle-lander
Merge into lp-tarmac

Revision history for this message

Diogo Matsubara (matsubara) wrote on 2010-12-14:

On Mon, Dec 13, 2010 at 3:03 PM, Gary Poster <email address hidden> 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.

I did this one by reusing Tarmac's PID_FILE in the config.

> 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.

Done. SAs need to change the tarmac config file to look like:

allowed_lock_period = X where X is the amount of seconds allowed for
the lock to be considered OK.

If the lock file is in place for less than the amount configured,
tarmac will return and log the amount. If the lock file is in place
for more than the amount configured, tarmac will return an error and
show the PID and the lock file age.

> 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.

Done. It'll log the lock file age and pid as explained above.

> 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.

Didn't do this one.

>
> 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.

Done.

>
> 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.

Done.

Thanks for the review.
--
Diogo M. Matsubara

On Mon, Dec 13, 2010 at 3:03 PM, Gary Poster <gary.poster@canonical.com> 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.

I did this one by reusing Tarmac's PID_FILE in the config.

>  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.

Done. SAs need to change the tarmac config file to look like:

allowed_lock_period = X where X is the amount of seconds allowed for
the lock to be considered OK.

>  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.

Done. It'll log the lock file age and pid as explained above.

>  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.

Didn't do this one.

>
> 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.

Done.

>
> 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.

Done.

Thanks for the review.
-- 
Diogo M. Matsubara

« Back to merge proposal