1218457 - Allow localconfig to override (force) certain data/params values

Status: RESOLVED FIXED

Description

[Dylan Hardison [:dylan] (he/him)]

For the AWS migration, we need to override a few things in localconfig.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

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

[Philippe M. Chiasson (:gozer)]

(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

[David Lawrence [:dkl]]

(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

[Dylan Hardison [:dylan] (he/him)]

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

[Dylan Hardison [:dylan] (he/him)]

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

[Dylan Hardison [:dylan] (he/him)]

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

[David Lawrence [:dkl]]

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

[Dylan Hardison [:dylan] (he/him)]

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

[David Lawrence [:dkl]]

Comment on attachment 8681487 [details] [diff] [review]
1218457_4.patch

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

r=dkl

Comment 10

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   f03cb21..5852295  master -> master