372762 - Bootstrap - polish notification support

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[J. Paul Reed [:preed]]

This bug encompasses two issues with Bootstrap's notification support:

1. Need to polish the content of the messages, so we know which host is reporting in; we may want to include the logs (or parts of the logs) for steps?

2. Need to polish the mail handling, so lack of cc list is not a failure, etc.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: use hostname in bootstrap email

Use hostname in bootstrap email. This depends on the Config->System() method implemented in bug 372757 (which is still awaiting review).

Comment 2

[J. Paul Reed [:preed]]

Comment on attachment 259044 [details] [diff] [review]
use hostname in bootstrap email

I don't see a SystemInfo() function in Bootstrep::Config(); is part of the patch missing?

Comment 3

[J. Paul Reed [:preed]]

Comment on attachment 259044 [details] [diff] [review]
use hostname in bootstrap email

Didn't read your previous comment; looks fine.

Comment 4

[Robert Helmer [:rhelmer]]

Landed:

Checking in release;
/cvsroot/mozilla/tools/release/release,v  <--  release
new revision: 1.9; previous revision: 1.8
done
Checking in Bootstrap/Step.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step.pm,v  <--  Step.pm
new revision: 1.11; previous revision: 1.10
done

Comment 6

[Mike Schroepfer]

Should this be closed?

Comment 7

[Robert Helmer [:rhelmer]]

No, part #2 in comment 0 still needs to be cleaned up; bug 375587 is evidence of this. That said, it's polish IMHO (as there's a known workaround) so it's not at the top of my list.

I do intend to fix it though.

Comment 8

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

needed for "release automation", hence marking as critical.

Comment 9

[Robert Helmer [:rhelmer]]

Attachment: cleanup error handling, make blat handle cc

cf reported this over in bug 375587; blat should handle cc (duh), and errors about blat should call it "blat", not "sendmail".

Comment 10

[Robert Helmer [:rhelmer]]

Attachment: lack of cc list ok

As per part 2 of comment #0, cc list should be optional not required.

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 268575 [details] [diff] [review]
cleanup error handling, make blat handle cc

While I like the changes you've made, I did some testing and the blat part doesn't work so well.

On fx-win32-tbox (blat v2.5.0), you have to go
 ... -to [e-mail] -cc [e-mail],[e-mail]

On pacifica-vm, the installed v2.2.2 of blat doesn't support being on the end of a pipe, eg 
 cat somefile | blat -to victim1 -subject "lolcat says hai"
fails. We might have to think about updating blat on the windows boxes.

Comment 12

[Robert Helmer [:rhelmer]]

(In reply to comment #11)
> (From update of attachment 268575 [details] [diff] [review])
> While I like the changes you've made, I did some testing and the blat part
> doesn't work so well.
> 
> On fx-win32-tbox (blat v2.5.0), you have to go
>  ... -to [e-mail] -cc [e-mail],[e-mail]
> 
> On pacifica-vm, the installed v2.2.2 of blat doesn't support being on the end
> of a pipe, eg 
>  cat somefile | blat -to victim1 -subject "lolcat says hai"
> fails. We might have to think about updating blat on the windows boxes.

Thanks, I thought I had tested this.. I'll note the blat version this time.

We could get around the pipe problem by writing to a temp file and then calling blat on that.. this is what tinderbox does, and it seems to work on all the machines.

Comment 13

[J. Paul Reed [:preed]]

Comment on attachment 268576 [details] [diff] [review]
lack of cc list ok


>-        my $cc = $conf->Get(var => 'cc');
>+        my $cc = undef;

Why do we still need the $cc var?

I don't see it referenced anywhere else.

Other than that, looks good.

Comment 14

[Robert Helmer [:rhelmer]]

Attachment: do not initialize $cc

Thanks for pointing that out.. I noticed that there was a bug in this patch actually, @ccList would have been overridden!

This patch removes $cc and the old @ccList.

Comment 15

[Robert Helmer [:rhelmer]]

Checking in release;
/cvsroot/mozilla/tools/release/release,v  <--  release
new revision: 1.12; previous revision: 1.11
done

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

hey rob, what about the changes to MozBuild/Util.pm in attachment 268575 [details] [diff] [review] ? Did you decide fix the the blat arguments problem later ?

Comment 17

[Robert Helmer [:rhelmer]]

(In reply to comment #16)
> hey rob, what about the changes to MozBuild/Util.pm in attachment 268575 [details] [diff] [review] ? Did
> you decide fix the the blat arguments problem later ?

Right, sorry; reopening to resolve the issues in comment #11.

Comment 18

[Robert Helmer [:rhelmer]]

cf and I discussed in irc; it seems like the simplest solution here is the one from comment 12; that is, write the message to a temp file, then call sendmail or blat on that temp file (instead of using a pipe). That has worked fine for tinderbox for years regardless of blat version.

Comment 19

[Robert Helmer [:rhelmer]]

Attachment: use tempfile for email instead of pipe

Putting this through testing now; the point is that this should work with all versions of blat, older versions don't support pipes.

Comment 20

[Robert Helmer [:rhelmer]]

Attachment: simpler, and correct for sendmail at least.

Comment 21

[Robert Helmer [:rhelmer]]

Attachment: work with blat.exe regardless of version

Comment 22

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 273689 [details] [diff] [review]
work with blat.exe regardless of version

>+        if ($rv->{'timedOut'} || $rv->{'exitValue'} != 0) {
>+          die("DownloadFile(): FAILED: $rv->{'exitValue'}," .
>+              " output: $rv->{'output'}\n");
>+        }

r+ with s/DownloadFile/Email/. Time to move on to Czech Republic, mmmmm Pilsner. :-)

Comment 23

[Robert Helmer [:rhelmer]]

(In reply to comment #22)
> (From update of attachment 273689 [details] [diff] [review])
> >+        if ($rv->{'timedOut'} || $rv->{'exitValue'} != 0) {
> >+          die("DownloadFile(): FAILED: $rv->{'exitValue'}," .
> >+              " output: $rv->{'output'}\n");
> >+        }
> 
> r+ with s/DownloadFile/Email/. Time to move on to Czech Republic, mmmmm
> Pilsner. :-)

Copy and paste code strikes again! Thanks Nick!

I went ahead and added the method name to all die() statements in the Email() method while I was at it.

Comment 24

[Robert Helmer [:rhelmer]]

Attachment: patch as landed

Checking in MozBuild/Util.pm;
/cvsroot/mozilla/tools/release/MozBuild/Util.pm,v  <--  Util.pm
new revision: 1.18; previous revision: 1.17
done