274650 - checksetup flag fixup code outputs without checking silent mode

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Myk Melez [:myk] [@mykmelez]]

In silent mode checksetup should be silent, but the fix for bug 274428, which
corrects flag type names with commas or spaces, doesn't pay attention to the
$silent boolean.  It should do so by appending "unless $silent" to print statements.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Right. I should have observed that.

Comment 2

[Nick Barnes]

My mistake; I'm not used to $silent yet.  Sorry.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 168761 [details] [diff] [review]
Patch

Looks good, r=myk.  Thanks for the quick patch!

Comment 4

[Dave Miller [:justdave]]

Comment on attachment 168761 [details] [diff] [review]
Patch

$silent only applies to progress information.  Errors and warnings and
announcements of changes to the database *SHOULD* be displayed, even in silent
mode, which applies to most of this text.  The point of silent mode was to
prevent needless cron mail when running checksetup.pl from a cron job.	If
changes are made to the database, I *want* mail from cron telling me about it.

Comment 5

[Dave Miller [:justdave]]

To clarify that, any text which is printed regardless every time checksetup.pl
is run should use "unless $silent".  Any text which announces an error being
corrected, or information that the operator should know, should be displayed
even if in silent mode, provided that the message will go away either as a
result of the change or when the problem being reported to the operator is
corrected.

Comment 6

[Marc Schumann [:Wurblzap]]

Attachment: Breaking silence when fixing things

This patch should be good, then.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 168864 [details] [diff] [review]
Breaking silence when fixing things

>-        print "... done.\n";
>+        print "... done.\n" unless $silent;

This statement is part of the block that fixes bad flag names, so it shouldn't
ever be silent just as the other print statements in that block aren't ever
silent, per Dave's sensible criteria.

Otherwise the patch looks great.

Comment 8

[Marc Schumann [:Wurblzap]]

Attachment: Breaking silence when successfully completing repairs, too

I was leaning towards filing this line as "stuff that may be silenced without
loss", but it makes sense to bless it as part of the "information that the
operator should know".

Comment 9

[Jacob Steenhagen]

Comment on attachment 169061 [details] [diff] [review]
Breaking silence when successfully completing repairs, too

Looks sane... just one question:

>-                        print "  ... last attempt as \"$lastchanceflagname\" still failed.'\n";
>-                        print "Rename the flag by hand and run checksetup.pl again.\n";
>+                        print "  ... last attempt as \"$lastchanceflagname\" still failed.'\n",
>+                              "Rename the flag by hand and run checksetup.pl again.\n";

Why include that? it looks like it was just a relic from when other things were
being changed.

Comment 10

[Dave Miller [:justdave]]

let's get this freaking checked in.  I'm tired of the thrice hourly emails from
landfill ;)

Comment 11

[Vlad Dascalu]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.320; previous revision: 1.319
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.18; previous revision: 1.289.2.17
done