Bootstrap - polish notification support

RESOLVED FIXED

Status

defect
P2
normal
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: preed, Assigned: rhelmer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ETA July 24th)

Attachments

(5 attachments, 3 obsolete attachments)

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.
Use hostname in bootstrap email. This depends on the Config->System() method implemented in bug 372757 (which is still awaiting review).
Assignee: build → rhelmer
Status: NEW → ASSIGNED
Attachment #259044 - Flags: review?(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 on attachment 259044 [details] [diff] [review]
use hostname in bootstrap email

Didn't read your previous comment; looks fine.
Attachment #259044 - Flags: review?(preed) → review+
Whiteboard: waiting to land with 372757
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
Whiteboard: waiting to land with 372757
Duplicate of this bug: 375587
Should this be closed?
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.
needed for "release automation", hence marking as critical.
Severity: normal → critical
Severity: critical → normal
Priority: -- → P3
cf reported this over in bug 375587; blat should handle cc (duh), and errors about blat should call it "blat", not "sendmail".
Attachment #268575 - Flags: review?(nrthomas)
Posted patch lack of cc list ok (obsolete) — Splinter Review
As per part 2 of comment #0, cc list should be optional not required.
Attachment #268576 - Flags: review?(preed)
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 main_victim@gmail.com -cc victim1@foo.com,victim2@bar.com

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.
Attachment #268575 - Flags: review?(nrthomas) → review-
(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 main_victim@gmail.com -cc victim1@foo.com,victim2@bar.com
> 
> 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 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.
Attachment #268576 - Flags: review?(preed) → review+
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.
Attachment #268576 - Attachment is obsolete: true
Attachment #269770 - Flags: review?(preed)
Attachment #269770 - Flags: review?(preed) → review+
Checking in release;
/cvsroot/mozilla/tools/release/release,v  <--  release
new revision: 1.12; previous revision: 1.11
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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 ?
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: rhelmer → build
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Assignee: build → rhelmer
Status: ASSIGNED → NEW
Putting this through testing now; the point is that this should work with all versions of blat, older versions don't support pipes.
Attachment #273523 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: ETA July 24th
Attachment #273539 - Attachment is obsolete: true
Attachment #273689 - Flags: review?(nrthomas)
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. :-)
Attachment #273689 - Flags: review?(nrthomas) → review+
(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.
Checking in MozBuild/Util.pm;
/cvsroot/mozilla/tools/release/MozBuild/Util.pm,v  <--  Util.pm
new revision: 1.18; previous revision: 1.17
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.