Closed
Bug 372762
Opened 18 years ago
Closed 17 years ago
Bootstrap - polish notification support
Categories
(Release Engineering :: General, defect, P2)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: preed, Assigned: rhelmer)
References
Details
(Whiteboard: ETA July 24th)
Attachments
(5 files, 3 obsolete files)
1.74 KB,
patch
|
preed
:
review+
|
Details | Diff | Splinter Review |
1.53 KB,
patch
|
nthomas
:
review-
|
Details | Diff | Splinter Review |
849 bytes,
patch
|
preed
:
review+
|
Details | Diff | Splinter Review |
1.66 KB,
patch
|
nthomas
:
review+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
Use hostname in bootstrap email. This depends on the Config->System() method implemented in bug 372757 (which is still awaiting review).
Reporter | ||
Comment 2•18 years ago
|
||
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?
Reporter | ||
Comment 3•18 years ago
|
||
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+
Assignee | ||
Updated•18 years ago
|
Whiteboard: waiting to land with 372757
Assignee | ||
Comment 4•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Whiteboard: waiting to land with 372757
Comment 6•18 years ago
|
||
Should this be closed?
Assignee | ||
Comment 7•18 years ago
|
||
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•17 years ago
|
||
needed for "release automation", hence marking as critical.
Severity: normal → critical
Updated•17 years ago
|
Severity: critical → normal
Priority: -- → P3
Assignee | ||
Comment 9•17 years ago
|
||
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)
Assignee | ||
Comment 10•17 years ago
|
||
As per part 2 of comment #0, cc list should be optional not required.
Attachment #268576 -
Flags: review?(preed)
Comment 11•17 years ago
|
||
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-
Assignee | ||
Comment 12•17 years ago
|
||
(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.
Reporter | ||
Comment 13•17 years ago
|
||
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+
Assignee | ||
Comment 14•17 years ago
|
||
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)
Reporter | ||
Updated•17 years ago
|
Attachment #269770 -
Flags: review?(preed) → review+
Assignee | ||
Comment 15•17 years ago
|
||
Checking in release;
/cvsroot/mozilla/tools/release/release,v <-- release
new revision: 1.12; previous revision: 1.11
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 16•17 years ago
|
||
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 ?
Assignee | ||
Comment 17•17 years ago
|
||
(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 → ---
Assignee | ||
Comment 18•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•17 years ago
|
Assignee: build → rhelmer
Status: ASSIGNED → NEW
Assignee | ||
Comment 19•17 years ago
|
||
Putting this through testing now; the point is that this should work with all versions of blat, older versions don't support pipes.
Assignee | ||
Comment 20•17 years ago
|
||
Attachment #273523 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: ETA July 24th
Assignee | ||
Comment 21•17 years ago
|
||
Attachment #273539 -
Attachment is obsolete: true
Attachment #273689 -
Flags: review?(nrthomas)
Comment 22•17 years ago
|
||
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+
Assignee | ||
Comment 23•17 years ago
|
||
(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.
Assignee | ||
Comment 24•17 years ago
|
||
Checking in MozBuild/Util.pm;
/cvsroot/mozilla/tools/release/MozBuild/Util.pm,v <-- Util.pm
new revision: 1.18; previous revision: 1.17
done
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•