Last Comment Bug 1218457 - Allow localconfig to override (force) certain data/params values
: Allow localconfig to override (force) certain data/params values
Status: RESOLVED FIXED
:
Product: bugzilla.mozilla.org
Classification: Other
Component: General (show other bugs)
: Production
: Unspecified Unspecified
-- normal (vote)
: ---
Assigned To: Dylan Hardison [:dylan]
:
:
Mentors:
Depends on:
Blocks: 1250211
  Show dependency treegraph
 
Reported: 2015-10-26 10:29 PDT by Dylan Hardison [:dylan]
Modified: 2016-02-22 09:29 PST (History)
2 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1218457_2.patch (19.91 KB, patch)
2015-10-28 18:45 PDT, Dylan Hardison [:dylan]
no flags Details | Diff | Splinter Review
1218457_3.patch (19.91 KB, patch)
2015-10-28 19:04 PDT, Dylan Hardison [:dylan]
dkl: review-
Details | Diff | Splinter Review
1218457_4.patch (7.85 KB, patch)
2015-10-30 13:55 PDT, Dylan Hardison [:dylan]
dkl: review+
Details | Diff | Splinter Review

Description User image Dylan Hardison [:dylan] 2015-10-26 10:29:43 PDT
For the AWS migration, we need to override a few things in localconfig.
Comment 1 User image Dylan Hardison [:dylan] 2015-10-26 14:21:33 PDT
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?
Comment 2 User image Philippe M. Chiasson (:gozer) 2015-10-26 16:21:43 PDT
(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/
Comment 3 User image David Lawrence [:dkl] 2015-10-26 17:13:52 PDT
(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
Comment 4 User image Dylan Hardison [:dylan] 2015-10-28 14:58:44 PDT
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.
Comment 5 User image Dylan Hardison [:dylan] 2015-10-28 18:45:47 PDT
Created attachment 8680405 [details] [diff] [review]
1218457_2.patch

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.
Comment 6 User image Dylan Hardison [:dylan] 2015-10-28 19:04:11 PDT
Created attachment 8680422 [details] [diff] [review]
1218457_3.patch

Noticed a quite severe bug in the previous patch. let's go with this one for review. :)
Comment 7 User image David Lawrence [:dkl] 2015-10-29 14:43:12 PDT
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 %]">
Comment 8 User image Dylan Hardison [:dylan] 2015-10-30 13:55:21 PDT
Created attachment 8681487 [details] [diff] [review]
1218457_4.patch

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.
Comment 9 User image David Lawrence [:dkl] 2015-10-30 14:28:46 PDT
Comment on attachment 8681487 [details] [diff] [review]
1218457_4.patch

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

r=dkl
Comment 10 User image Dylan Hardison [:dylan] 2015-10-30 18:07:32 PDT
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   f03cb21..5852295  master -> master

Note You need to log in before you can comment on or make changes to this bug.