Merge lp://staging/~matsubara/tarmac/bundle-lander into lp://staging/~launchpad/tarmac/lp-tarmac

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.

And by the way, very nice branch. :-) Thank you.

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

Hi Gary,

based in our conversation on IRC I added tests to the lock file mechanism and to the functions in the pidfile.py module. I removed an XXX in the bundle merge command test to make it more complete.

Let me know what you think and if this is ok to be merged.

Thanks for the detailed review and help.

Hi Diogo. We talked about a couple of things that will probably be important for merging this into tarmac trunk, but since we are not doing that now, I'm not worrying about it yet. Thank you!

Here are the notes I have that might be pertinent when we consider merging this to tarmac trunk.

 - We are reusing some sort of tarmac pid configuration value that didn't seem to actually be used in tarmac. Therefore, we are not sure if what we are doing makes sense in the larger context.

 - We are only using the lock for certain commands, not help. Does that make sense, or should we make the lock pervasive, or pervasive for everything other than help? I don't know, and it's not necessary to figure out the answer yet, but it will be when a merge is possible.

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.