Closed
Bug 1296831
Opened 8 years ago
Closed 8 years ago
allow answers file to override new install defaults
Categories
(Bugzilla :: Installation & Upgrading, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 6.0
People
(Reporter: Frank, Assigned: Frank)
Details
Attachments
(1 file, 2 obsolete files)
1013 bytes,
patch
|
dylan
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/601.7.7 (KHTML, like Gecko) Version/9.1.2 Safari/601.7.7 Steps to reproduce: create a answer file and include the following line $answer{'use_email_as_login'} = '1'; run ./checksetup.pl ./answers Actual results: you get the following output: Initializing "Product/Component Changes" email_setting ... Initializing "Dependency Tree Changes" email_setting ... Deriving regex group memberships... Login names may not contain the "@" sign unless you are setting your login name to be identical to your email address. and in data/params.json you find the line "use_email_as_login" : 0, Expected results: creation without an error and data/params.json with the following line "use_email_as_login" : 1,
Updated•8 years ago
|
Assignee: installation → dylan
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → Bugzilla 6.0
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8783195 -
Flags: review?(gerv)
Updated•8 years ago
|
Assignee: dylan → Frank
Comment 2•8 years ago
|
||
Comment on attachment 8783195 [details] [diff] [review] patch Review of attachment 8783195 [details] [diff] [review]: ----------------------------------------------------------------- r=dylan
Attachment #8783195 -
Flags: review?(gerv) → review+
Comment 3•8 years ago
|
||
To git@github.com:bugzilla/bugzilla 30b5fd6..a8824f3 master -> master
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 4•8 years ago
|
||
Comment on attachment 8783195 [details] [diff] [review] patch > if ($new_install) { > $param->{'or_groups'} = 1; >- $param->{'use_email_as_login'} = 0; >+ $param->{'use_email_as_login'} = 0 unless exists $param->{'use_email_as_login'}; > } This patch is wrong and is not the correct fix. $param->{'use_email_as_login'} is always defined, even for new installations, and so this code will no longer be triggered. Please backout this patch asap. You must fix the previous if-else-end block.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Frédéric Buclin from comment #4) > Comment on attachment 8783195 [details] [diff] [review] > patch > > > if ($new_install) { > > $param->{'or_groups'} = 1; > >- $param->{'use_email_as_login'} = 0; > >+ $param->{'use_email_as_login'} = 0 unless exists $param->{'use_email_as_login'}; > > } > > This patch is wrong and is not the correct fix. > $param->{'use_email_as_login'} is always defined, even for new > installations, and so this code will no longer be triggered. Please backout > this patch asap. You must fix the previous if-else-end block. I do not see how I can fix this in the previous if-else-end block. For a $new_install we always overwrite the settings from the previous if-else-end block.
Comment 6•8 years ago
|
||
I guess the effective behavior is = 1 at the moment. I'm not opposed to it defauling to 1 anyway, as the other situation is a regression on the behavior of every released version of bugzilla. Many sites may want login names that are not emails, but that should be opt in (even for $new_install, as that ends up being true in more modern deployment patterns).
Status: RESOLVED → REOPENED
Flags: needinfo?(Frank)
Resolution: FIXED → ---
Comment 7•8 years ago
|
||
is my supposition about $new_install being effectively wrong correct, Frank?
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Dylan William Hardison [:dylan] from comment #7) > is my supposition about $new_install being effectively wrong correct, Frank? Yes, I did some more research and now think that we can fix this with a change in Bugzilla/Config/Auth.pm (line 91 to 96) sub get_param_list defines { name => 'use_email_as_login', type => 'b', default => '1', onchange => \&change_use_email_as_login }, but for new installations we change this in Bugzilla/Config.pm to '0'. Why we not change Bugzilla/Auth.pm line 94 from default => '1', to default => '0', Thoughts?
Flags: needinfo?(Frank)
Attachment #8783794 -
Flags: review?
Comment 9•8 years ago
|
||
(In reply to Frank Becker from comment #0) > Steps to reproduce: > > create a answer file and include the following line > $answer{'use_email_as_login'} = '1'; Just to check: what makes you think this should work? Is it supposed to be possible to put any pref into this file and have it work? The default value of use_email_as_login is 1 because that's what it should be for upgrades. But there is code in Bugzilla/Config.pm to set use_email_as_login to 0 for new installs: if ($new_install) { $param->{'or_groups'} = 1; $param->{'use_email_as_login'} = 0; } Is that perhaps the code you are looking for? Gerv
Comment 10•8 years ago
|
||
(In reply to Frank Becker from comment #8) > Yes, I did some more research and now think that we can fix this with a > change in Bugzilla/Config/Auth.pm (line 91 to 96) No, this code must be left alone. As gerv said in comment 9, on upgrades we want email = login because that's the current behavior of older installations, and we don't want to suddenly change a rule which didn't exist before the upgrade. But for *new* installations, we want the email != login feature enabled by default. As I said in comment 4, the right fix is to move if ($new_install) { $param->{'or_groups'} = 1; $param->{'use_email_as_login'} = 0; } inside the previous if-else-end block. This block becomes the 2nd ELSIF block: + my %new_defaults = (or_groups => 1, use_email_as_login => 0); foreach (...) { if (exists $new_params{$name}) { $param->{$name} = $new_params{$name}; } elsif (exists $answer->{$name}) { $param->{$name} = $answer->{$name}; } + elsif ($new_install and defined $new_defaults{$name}) { + $param->{$name} = $new_defaults{$name}; + } else { $param->{$name} = $item->{'default'}; } } - if ($new_install) { - $param->{'or_groups'} = 1; - $param->{'use_email_as_login'} = 0; - }
Comment 11•8 years ago
|
||
Patch backed out. To github.com:bugzilla/bugzilla.git a8824f3..3987a28 master -> master I would still find it acceptable to not turn on this feature by default for anyone. I don't think Frank is working on a new install and I suspect more people will be tripped up by this behavior.
Comment 12•8 years ago
|
||
I think we definitely want this feature on by default for new installs. It's a significant privacy improvement. Gerv
Comment 13•8 years ago
|
||
(In reply to Dylan William Hardison [:dylan] from comment #11) > I don't think Frank is working on a new install He is, else the code in |if $new_install| wouldn't be triggered.
Assignee | ||
Comment 14•8 years ago
|
||
This patch has the changes from comment #10 (In reply to Gervase Markham [:gerv] from comment #9) > (In reply to Frank Becker from comment #0) > > Steps to reproduce: > > > > create a answer file and include the following line > > $answer{'use_email_as_login'} = '1'; > > Just to check: what makes you think this should work? Is it supposed to be > possible to put any pref into this file and have it work? > > The default value of use_email_as_login is 1 because that's what it should > be for upgrades. But there is code in Bugzilla/Config.pm to set > use_email_as_login to 0 for new installs: > > if ($new_install) { > $param->{'or_groups'} = 1; > $param->{'use_email_as_login'} = 0; > } > > Is that perhaps the code you are looking for? > > Gerd Yes when you setup an new install you always get use_email_as_login = 0. When you add $answer{'use_email_as_login'} = '1'; that did not change. (In reply to Frédéric Buclin from comment #13) > (In reply to Dylan William Hardison [:dylan] from comment #11) > > I don't think Frank is working on a new install > > He is, else the code in |if $new_install| wouldn't be triggered. Yes it is a new install. I use http://git.eclipse.org/c/mylyn/org.eclipse.mylyn.tasks.git/tree/org.eclipse.mylyn.bugzilla.releng to setup the vagrant mylyn test instances or the instances at http://mylyn.org
Attachment #8783794 -
Attachment is obsolete: true
Attachment #8783794 -
Flags: review?
Attachment #8784018 -
Flags: review?
Comment 15•8 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #9) > (In reply to Frank Becker from comment #0) > > Steps to reproduce: > > > > create a answer file and include the following line > > $answer{'use_email_as_login'} = '1'; > > Just to check: what makes you think this should work? Is it supposed to be > possible to put any pref into this file and have it work? Still looking for an answer to this question, if only to enlighten me about how this undocumented part of our install system works... Gerv
Comment 16•8 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #15) > > Just to check: what makes you think this should work? Is it supposed to be > > possible to put any pref into this file and have it work? > > Still looking for an answer to this question, if only to enlighten me about > how this undocumented part of our install system works... This is documented here: https://www.bugzilla.org/docs/5.0/en/html/integrating/api/checksetup.html#RUNNING_CHECKSETUP_NON-INTERACTIVELY The relevant part is: "Any localconfig variable or parameter can be specified as above."
Comment 17•8 years ago
|
||
Comment on attachment 8784018 [details] [diff] [review] patch V4 Review of attachment 8784018 [details] [diff] [review]: ----------------------------------------------------------------- r=dylan this also lets the answers file override or_groups being default. My suggested fix would have been: $param->{'use_email_as_login'} = 0 unless exists $answer->{'use_email_as_login'}; which wouldn't have fixed the other (unreported) problem.
Attachment #8784018 -
Flags: review? → review+
Updated•8 years ago
|
Summary: answer use_email_as_login not used → allow answers file to override new install defaults
Comment 18•8 years ago
|
||
To github.com:bugzilla/bugzilla.git 4da9ab1..0ee7422 master -> master
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Attachment #8783195 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•