Closed Bug 936468 Opened 6 years ago Closed 6 years ago

Move OS: Windows 8 Metro to Windows 8.1

Categories

(bugzilla.mozilla.org :: Administration, task, P2)

Production
x86_64
All

Tracking

()

RESOLVED FIXED

People

(Reporter: jimm, Assigned: dylan)

Details

Attachments

(1 file, 3 obsolete files)

This was originally created to isolate metro related bugs from product firefox. Later added a new product, so this isn't needed anymore. We would like to delete this os moving any bug under it to os: Windows 8.1 (hopefully without generating any bugspam?).

This change was discussed on the metro public mailing list.
i've updated the field value so it's no longer an option.
we'll have to write a script to update the 1962 bugs currently using that o/s.
Assignee: nobody → dylan
Attached patch bug-936468-v1.patch (obsolete) — Splinter Review
This is decidedly a one-off script. Current behavior is to reset the OS field to 'Other', I'm not sure what else would be defined behavior.
Attachment #8439730 - Flags: review?(glob)
Status: NEW → ASSIGNED
Comment on attachment 8439730 [details] [diff] [review]
bug-936468-v1.patch

Review of attachment 8439730 [details] [diff] [review]:
-----------------------------------------------------------------

::: contrib/reorg-tools/remove-os.pl
@@ +8,5 @@
> +
> +use 5.10.1;
> +use strict;
> +use warnings;
> +use lib qw(. lib);

while most reorg script don't current do this, i think it's nicer to allow these scripts to work regardless of the cwd:

use FindBin '$RealBin';
use lib "$ReadBin/../..", "$RealBin/../../lib";

@@ +32,5 @@
> +
> +$dbh->bz_start_transaction;
> +
> +my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +my $field     = Bugzilla::Field->new({name => 'op_sys', cache => 1});

nit: add spaces after { and before }

i'd like to see a sanity check here that default_os exists in the db (stranger things have happened!).
check the result of Bugzilla::Field::Choice->type($field)->match({ value => $DEFAULT_OS })

@@ +35,5 @@
> +my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +my $field     = Bugzilla::Field->new({name => 'op_sys', cache => 1});
> +
> +my $bugs = $dbh->selectall_arrayref(q{SELECT bug_id FROM bugs WHERE bugs.op_sys = ?},
> +    { Slice => {} }, $os);

rather than slicing to a single element hashref, use selectcol_arrayref

@@ +42,5 @@
> +    my $bug_id = $bug->{bug_id};
> +    warn "Removing OS '$os' from $bug_id...\n";
> +    $dbh->do(q{INSERT INTO bugs_activity(bug_id, who, bug_when, fieldid, removed, added)
> +               VALUES (?, ?, ?, ?, ?, ?)},
> +                undef, $bug_id, 1, $timestamp, $field->id, $os, $DEFAULT_OS);

don't hard code the user-id.  load the nobody user explicitly with $user = Bugzilla::User->check({ name => 'nobody@mozilla.org' })

@@ +63,5 @@
> +    remove-os.pl --os 'Windows 8 Metro'
> +
> +=head1 AUTHOR
> +
> +Dylan William Hardison <dylan@mozilla.com>

remove the author section
Attachment #8439730 - Flags: review?(glob) → review-
Attached patch bug-936468-v2.patch (obsolete) — Splinter Review
And here is something I was working on when I was sick. Note that using FindBin under taint mode requires untainting $RealBin.
Attachment #8439730 - Attachment is obsolete: true
Attachment #8454233 - Flags: review?(glob)
Attached patch bug-936468-v2.1.patch (obsolete) — Splinter Review
Ignore the previous version, pretty sure I made a mistake there.
Attachment #8454233 - Attachment is obsolete: true
Attachment #8454233 - Flags: review?(glob)
Attachment #8454235 - Flags: review?(glob)
Comment on attachment 8454235 [details] [diff] [review]
bug-936468-v2.1.patch

Review of attachment 8454235 [details] [diff] [review]:
-----------------------------------------------------------------

::: contrib/reorg-tools/remove-os.pl
@@ +1,1 @@
> +#!/usr/bin/perl -wT

nit: don't need both -w and "use warnings"; remove -w from here.

@@ +11,5 @@
> +use warnings;
> +
> +use FindBin '$RealBin';
> +BEGIN { ($RealBin) = $RealBin =~ /^(.+)$/ }
> +use lib "$RealBin/../..", "$RealBin/../../lib";

as it's a command line tool with a single source of user input, i'm not convinced the acrobatics required to work around taint issues are worth enabling of taint checking.
i'll leave it up to you if you want to leave taint mode enabled.

@@ +34,5 @@
> +trick_taint($os);
> +
> +my $check_default_os = Bugzilla::Field::Choice->type('op_sys')->match({ value => $DEFAULT_OS });
> +die "This should not happen: Cannot remove $os because $DEFAULT_OS doesn't exist.\n"
> +    unless @$check_default_os == 1;

it's probably a good idea to also check that $os is a valid op_sys.
currently if you pass in an invalid OS no output is produced by the script, which is confusing.

nit: "This should not happen: " isn't necessary for error messages... errors in general shouldn't happen :)

@@ +40,5 @@
> +my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +my $field     = Bugzilla::Field->check({ name => 'op_sys', cache => 1 });
> +my $nobody    = Bugzilla::User->check({ name => 'nobody@mozilla.org', cache => 1 });
> +my $bug_ids   = $dbh->selectcol_arrayref(q{SELECT bug_id FROM bugs WHERE bugs.op_sys = ?}, undef, $os);
> +

please add a confirmation prompt here as this script could be destructive (eg. as per movebugs.pl)
Attachment #8454235 - Flags: review?(glob) → review-
Priority: -- → P2
Comment on attachment 8454235 [details] [diff] [review]
bug-936468-v2.1.patch

Review of attachment 8454235 [details] [diff] [review]:
-----------------------------------------------------------------

::: contrib/reorg-tools/remove-os.pl
@@ +23,5 @@
> +use Pod::Usage;
> +
> +Bugzilla->usage_mode(USAGE_MODE_CMDLINE);
> +
> +my $DEFAULT_OS = 'Other';

In the case of the metro bugs, I'd prefer they get moved to Windows 8.1. Otherwise I'm still going to have to spam the entire list updating to that. :/ Maybe this script should accept an optional command line param specifying the os to switch bug into?
(In reply to Jim Mathies [:jimm] from comment #7)
> In the case of the metro bugs, I'd prefer they get moved to Windows 8.1.
> Otherwise I'm still going to have to spam the entire list updating to that.
> :/ Maybe this script should accept an optional command line param specifying
> the os to switch bug into?

This would be a more useful script to have in the future, and is trivial to implement. Good call.
Summary: Remove OS: Windows 8 Metro → Move OS: Windows 8 Metro to Windows 8.1
While we had agreed this wouldn't need another round of review, given that the nature of the script changed slightly (movement rather than removal) I figured it warranted another r?
Attachment #8454235 - Attachment is obsolete: true
Attachment #8457737 - Flags: review?(glob)
Or, rather -- given that the nature of this script changed, it deserves additional sanity checking than would be warranted.
Comment on attachment 8457737 [details] [diff] [review]
bug-936468-v3.patch

Review of attachment 8457737 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: contrib/reorg-tools/move_os.pl
@@ +10,5 @@
> +use strict;
> +use warnings;
> +
> +use FindBin '$RealBin';
> +BEGIN { ($RealBin) = $RealBin =~ /^(.+)$/ }

you don't need to detaint $RealBin
Attachment #8457737 - Flags: review?(glob) → review+
To gitolite3@git.mozilla.org:webtools/bmo/bugzilla.git
   b1f98ae..10bd960  master -> master
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.