Closed
Bug 1218457
Opened 6 years ago
Closed 6 years ago
Allow localconfig to override (force) certain data/params values
Categories
(bugzilla.mozilla.org :: General, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dylan, Assigned: dylan)
References
Details
Attachments
(1 file, 2 obsolete files)
7.85 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
For the AWS migration, we need to override a few things in localconfig.
Assignee | ||
Comment 1•6 years ago
|
||
What I propose here is that the following parameters exist as variables in localconfig: - $memcached_servers - $shadowdbdb_{host,port,etc} - $inbound_proxies Then, we edit the code to look for Bugzilla->localconfig->{KEY} // Bugzilla->params->{KEY}. This would require changes in Bugzilla/DB.pm, Bugzilla/Memcached.pm, and Bugzilla/Util.pm. On the more long term look, I believe these parameters more belong to localconfig than data/params. One questuin for :gozer -- will the http proxy be different for each AWS webhead?
Flags: needinfo?(gozer)
Comment 2•6 years ago
|
||
(In reply to Dylan William Hardison [:dylan] from comment #1) > What I propose here is that the following parameters exist as variables in > localconfig: > > - $memcached_servers > - $shadowdbdb_{host,port,etc} > - $inbound_proxies Sounds good t o me. > Then, we edit the code to look for Bugzilla->localconfig->{KEY} // > Bugzilla->params->{KEY}. > This would require changes in Bugzilla/DB.pm, Bugzilla/Memcached.pm, and > Bugzilla/Util.pm. > > On the more long term look, I believe these parameters more belong to > localconfig than data/params. In general, I think there should be a distinction betwen 'operational' paramaters, i.e. the ones needed to run the software, vs. the 'application' paramaters, these that control how the application functions (default theme, search on/off, etc) > One questuin for :gozer -- will the http proxy be different for each AWS > webhead? No, the proxies are discovered via service discovery, so the url will always be like: http://proxy.service.consul:3128/
Flags: needinfo?(gozer)
Comment 3•6 years ago
|
||
(In reply to Philippe M. Chiasson (:gozer) from comment #2) > In general, I think there should be a distinction betwen 'operational' > paramaters, i.e. the ones needed to run the software, vs. the 'application' > paramaters, these that control how the application > functions (default theme, search on/off, etc) What if we used a params_* prefix to denote that it is an application config setting? then before we override we prepend the params_ to the key name. dkl
Assignee | ||
Comment 4•6 years ago
|
||
How this has ended up working is that I'm adding a single %override_param hash to localconfig, and then add a Bugzilla->get_param_with_override("foo") convenience wrapper that uses defined-or. Adding only a single variable means I only need to add one thing to the localconfig setup in checksetup.
Assignee | ||
Comment 5•6 years ago
|
||
Here's the patch. Note that I made some changes the admin params page so that we don't footgun ourselves with the few params that can be overriden.
Attachment #8680405 -
Flags: review?(dkl)
Assignee | ||
Comment 6•6 years ago
|
||
Noticed a quite severe bug in the previous patch. let's go with this one for review. :)
Attachment #8680405 -
Attachment is obsolete: true
Attachment #8680405 -
Flags: review?(dkl)
Attachment #8680422 -
Flags: review?(dkl)
Comment 7•6 years ago
|
||
Comment on attachment 8680422 [details] [diff] [review] 1218457_3.patch Review of attachment 8680422 [details] [diff] [review]: ----------------------------------------------------------------- Mostly nits. The patch would have been somewhat smaller if we had put the logic in Bugzilla->params and not used a new get_param_with_override. But since this is temporary until we permanently move the settings to localconfig, we will can leave it this way for now. Will r+ the next revision of this patch. ::: Bugzilla/Install/Localconfig.pm @@ +122,5 @@ > + memcached_namespace => undef, > + shadowdb => undef, > + shadowdbhost => undef, > + shadowdbport => undef, > + shadowdbsock => undef, nit: line up => ::: Bugzilla/Memcached.pm @@ +28,4 @@ > > # always return an object to simplify calling code when memcached is > # disabled. > + my $servers = Bugzilla->get_param_with_override("memcached_servers") // ''; The // '' are unnecessary as we fall back to default values in Bugzilla->get_param_with_override. ::: Bugzilla/Util.pm @@ +308,5 @@ > # Returns the real remote address of the client, > sub remote_ip { > my $remote_ip = $ENV{'REMOTE_ADDR'} || '127.0.0.1'; > + my $proxies = Bugzilla->get_param_with_override('inbound_proxies'); > + my @proxies = split(/[\s,]+/, $proxies); nit: not fond of two vars with the same name but different data types. How bout: my @proxies = split(/[\s,]+/, Bugzilla->get_param_with_override('inbound_proxies')); ::: template/en/default/admin/params/common.html.tmpl @@ +21,3 @@ > [%# INTERFACE: > + # panel: hash representing the current panel. > + #%] a lot of indentation issues in this file diff: We can actually shrink the changes needed here by just doing: diff --git a/template/en/default/admin/params/common.html.tmpl b/template/en/default/admin/params/common.html.tmpl index d86da0d..6458ba4 100644 --- a/template/en/default/admin/params/common.html.tmpl +++ b/template/en/default/admin/params/common.html.tmpl @@ -28,9 +28,19 @@ <dl> [% FOREACH param = panel.param_list %] - <dt id="[% param.name FILTER html %]_desc">[% param.name FILTER html %]</dt> + <dt id="[% param.name FILTER html %]_desc">[% param.name FILTER html %] + [% IF panel.param_override.${param.name}.defined %] + (localconfig override) + [% END %] + </dt> <dd>[% panel.param_descs.${param.name} FILTER none %] <p> + [% IF panel.param_override.${param.name}.defined %] + <input type="hidden" name="[% param.name FILTER html %] value="[% Param(param.name) FILTER html %]"> + <input type="text" size="80" readonly + id="[% param.name FILTER html %]" value="[% panel.param_override.${param.name} FILTER html %]"> + [% NEXT %] + [% END %] [% IF param.type == "t" %] <input type="text" size="80" name="[% param.name FILTER html %]" id="[% param.name FILTER html %]" value="[% Param(param.name) FILTER html %]">
Attachment #8680422 -
Flags: review?(dkl) → review-
Assignee | ||
Comment 8•6 years ago
|
||
I should have thought of using the NEXT trick. Much smaller patch that way. I thought of removing get_param_with_override. I think since it's easy to grep, and that we want to remove it later it's good to keep. Also it means we don't have to repeat the key name.
Attachment #8680422 -
Attachment is obsolete: true
Attachment #8681487 -
Flags: review?(dkl)
Comment 9•6 years ago
|
||
Comment on attachment 8681487 [details] [diff] [review] 1218457_4.patch Review of attachment 8681487 [details] [diff] [review]: ----------------------------------------------------------------- r=dkl
Attachment #8681487 -
Flags: review?(dkl) → review+
Assignee | ||
Comment 10•6 years ago
|
||
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git f03cb21..5852295 master -> master
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•