282686 - Multiple projects sharing the same Bugzilla codebase (different datastores).

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[guillomovitch]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041009 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041009 Firefox/1.0

The following patch allow to use an unique bugzilla installation for several
independant projects at once, using an environment variable to differenciate,
and different data directories for each. If this variable is empty, it defaults
to default behavior. 

Reproducible: Always

Comment 1

[guillomovitch]

Attachment: Patch against 2.18 version

Comment 2

[timeless]

Attachment: patch against tip, dropping an extra blank line and wrapping at 80cols

Comment 3

[Marc Schumann [:Wurblzap]]

Comment on attachment 174660 [details] [diff] [review]
Patch against 2.18 version

Intriguing idea.

>         return $template_include_path =
>+               ["$templatedir/$languages/$project",
>+                "$templatedir/$languages/custom",
>                 "$templatedir/$languages/extension",
>                 "$templatedir/$languages/default"];
>     }

This will issue "undefined value" warnings if the environment variable is not
set, won't it?
You could define $project to be '', of course, but I'd rather have the first
line ending in $project not put into the search path at all if the environment
variable is not set.

Comment 4

[guillomovitch]

Attachment: Updated patch

This new patch version fix the issue of undefined $project when defininf
templates path.

Comment 5

[Dave Miller [:justdave]]

nothing to approve yet until we have a reviewed patch.  This'll be sweet though,
so consider the idea blessed.

Comment 6

[guillomovitch]

Attachment: Complete patch against CVS head

Here is an updated patch against CVS head, so as to make review easier.

Comment 7

[Vlad Dascalu]

Comment on attachment 177560 [details] [diff] [review]
Complete patch against CVS head

Your patch contains TABs; Bugzilla's devels use spaces in their code; they have
runtests.pl to check for TABs and this will break the testing suite and set
their tree on red (burning).

> +	if ($project {

Unless I'm missing something, "(" doesn't have a pair to close it.

Comment 8

[Vlad Dascalu]

By the way, feel free to keep the review? requests only on valid patches (not
obsolete ones). I always assumed that this helps the reviewers get a grasp
faster of which patch really needs the review.

Comment 9

[Frédéric Buclin]

Comment on attachment 176067 [details] [diff] [review]
Updated patch

removing pending requests on obsolete patches...

Comment 10

[guillomovitch]

Attachment: Patch against CVS head with spaces instead of tabs

This version also fixe missing parenthesis issue.

Comment 11

[Marc Schumann [:Wurblzap]]

Comment on attachment 177649 [details] [diff] [review]
Patch against CVS head with spaces instead of tabs

>Index: webtools/bugzilla/Bugzilla/Config.pm
>+if ($ENV{PROJECT} && $ENV{PROJECT} =~ /^(\w+)$/) {

Please use quotes: $ENV{'PROJECT'}.

>+  $project = $1;
>+  $localconfig = "$libpath/localconfig.$project";
>+  $datadir = "$libpath/data/$project";
>+} else {
>+  $localconfig = "$libpath/localconfig";
>+  $datadir = "$libpath/data";
>+}

Please use 4-space indent as per
http://www.bugzilla.org/docs/developer.html#perl.

>Index: webtools/bugzilla/Bugzilla/Template.pm
>@@ -103,10 +103,21 @@
>     }
>     my $languages = trim(Param('languages'));
>     if (not ($languages =~ /,/)) {
>-        return $template_include_path =
>-               ["$templatedir/$languages/custom",
>-                "$templatedir/$languages/extension",
>-                "$templatedir/$languages/default"];
>+       if ($project) {
>+           $template_include_path = [
>+               "$templatedir/$languages/$project"

There's a trailing comma missing here -------------^

Fixing this last error, the patch works well. I'll r+ a new patch addressing
the above remarks.

When upgrading, it may be necessary to run checksetup.pl several times (for
each project) in order to get it to add new params into the then several
data/params:

>export PROJECT=customer_1
>./checksetup.pl
[...]
>export PROJECT=customer_n
>./checksetup.pl
[...]

We'll need to relnote the whole thing, and it needs documentation, too.

Comment 12

[guillomovitch]

Attachment: Corrected patch

Patch including various corrections asked by previous comment.

Comment 13

[Marc Schumann [:Wurblzap]]

Comment on attachment 178513 [details] [diff] [review]
Corrected patch

r=wurblzap per comment 11 and inspection of the interdiff to the new patch.

Comment 14

[Dave Miller [:justdave]]

hmm, nobody requested approval on this once it got reviewed?

holding approval now until we branch, we're too far into the freeze for this
now.  It hasn't bitrotted, has it?

Comment 15

[Dave Miller [:justdave]]

Comment on attachment 178513 [details] [diff] [review]
Corrected patch

>@@ -122,11 +133,27 @@
>         }
>     }
>     push(@usedlanguages, Param('defaultlanguage'));
>-    return $template_include_path =
>-        [map(("$templatedir/$_/custom",
>-              "$templatedir/$_/extension",
>-              "$templatedir/$_/default"),
>-             @usedlanguages)];
>+    if ($project) {
>+        $template_include_path = [
>+           map((
>+               "$templatedir/$_/project",
>+               "$templatedir/$_/custom",
>+               "$templatedir/$_/extension",
>+               "$templatedir/$_/default"
>+               ), @usedlanguages

Shouldn't this one be "$templatedir/$_/$project" ?

Comment 16

[Dave Miller [:justdave]]

by the way, trunk is open now, this will land as soon as we know it works :)

r+ can carry forward on a patch that addresses my review comment just now,
provided that it's been tested on the tip and still works (it's had a couple
months for things to happen elsewhere in the code that might interfere with it)

Comment 17

[guillomovitch]

Attachment: Corrected patch

>Shouldn't this one be "$templatedir/$_/$project" ?
Absolutly. I just corrected it.

Comment 18

[Marc Schumann [:Wurblzap]]

Comment on attachment 190049 [details] [diff] [review]
Corrected patch

Tested; works. (And with multiple languages now, too.)

Comment 19

[Frédéric Buclin]

Comment on attachment 190049 [details] [diff] [review]
Corrected patch

bitrotten

Comment 20

[guillomovitch]

Attachment: Updated patch

Comment 21

[Frédéric Buclin]

Comment on attachment 196246 [details] [diff] [review]
Updated patch

No code change; unbitrot only. Carrying forward Marc's r+.

Comment 22

[Frédéric Buclin]

Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.46; previous revision: 1.45
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.34; previous revision: 1.33
done

Comment 23

[Max Kanat-Alexander]

This feature desperately needs some documentation.

Comment 24

[Max Kanat-Alexander]

Added to the Bugzilla 2.22 Release Notes in bug 322960.

Comment 25

[guillomovitch]

Attachment: Feature documentation patch

This patch add the missing installation and usage instructions.

Comment 26

[victory <[e-mail]>]

Comment on attachment 215186 [details] [diff] [review]
Feature documentation patch

+<VirtualHost 212.85.153.228:80>
+    ServerName foo.bar.baz
+    SetEnv PROJECT foo
+    Alias /bugzilla /var/www/bugzilla
+</VirtualHost>

<> need to be escaped

Comment 27

[guillomovitch]

Attachment: Fixed documentation patch

Same patch with escaped content.

Comment 28

[timeless]

Comment on attachment 215237 [details] [diff] [review]
Fixed documentation patch

>+    <para>The previous instructions refered to a standard installation, with
>+      one unique bug base. However, you may want to host several distinct
>+      bases, without having to reinstall bugzilla several times. This is

having to /install/ /Bugzilla/ ...

I think. I could be wrong ....

>+      achievable through PROJECT environment variable. When accessed, Bugzilla

/possible/ /by using the/ PROJECT ...

>+      check for the existence of this variable, and if present, check for an

/checks/ for ... /checks/ for ...

>+      alternative configuration file named after its value, rather than the

I'm not fond of "after its value"

alternative configuration file by that name?

>+      default <filename>localconfig</filename>, in the same location. It also
>+      check for customized templates in a directory with the same name, at the

/checks/

>+      same level as <filename>default</filename> directory.

>+    <para>To setup an alternate base, just export PROJECT=foo before running

/set up/

>+      <command>checksetup.pl</command> for the first time. It will result in a
>+      file called <filename>foo</filename> instead of
>+      <filename>localconfig</filename>. Edit this file as previously, with

as /described above/ ?

>+      reference to new database, and re-run <command>checksetup.pl</command> to

to _a_ new database
?

>+      populate it. That's all.</para>

>+    <para>Now you have to configure the web server to pass this environment
>+      variable when accessed through an alternate URL, such as virtual host for

accessed /via/ an ...
? (ask someone else to confirm)

>+      instance:

>+    <para>Don't forget to also export this variable before accessing Bugzilla
>+      from other ways, such as cron tasks for instance.</para> 

/by other means/, such as cron tasks ||.

Comment 29

[guillomovitch]

Attachment: Corrected patch

This new version adresses previous comments.

Comment 30

[guillomovitch]

Attachment: Corrected patch

After checking my own code, I was wrong on alternative configuration file name.

Comment 31

[Teemu Mannermaa (:wicked)]

Comment on attachment 215792 [details] [diff] [review]
Corrected patch

Documentation reviews should be requested from the documentation team to notify correct people.

Comment 32

[Colin Ogilvie [:cso]]

Comment on attachment 215792 [details] [diff] [review]
Corrected patch

>Index: docs/xml/installation.xml

>+  <section>
>+    <title>Multiple bug bases on the same host</title>

What's a "Bug Base"?

>+    <para>The previous instructions refered to a standard installation, with
>+      one unique bug base. However, you may want to host several distinct
>+      bases, without having to install Bugzilla several times. This is possible
>+      by using the PROJECT environment variable. When accessed, Bugzilla checks
>+      for the existence of this variable, and if present, uses its value to
>+      check for an alternative configuration file named
>+      <filename>localconfig.&lt;PROJECT&gt;</filename> in the same location as
>+      the default one (<filename>localconfig</filename>). It also checks for
>+      customized templates in a directory named
>+      <filename>&lt;PROJECT&gt;</filename> in the same location as the default
>+      one (<filename>default</filename>).</para> 
>+
>+    <para>To set up an alternate base, just export PROJECT=foo before running
>+      <command>checksetup.pl</command> for the first time. It will result in a
>+      file called <filename>foo</filename> instead of
>+      <filename>localconfig</filename>. Edit this file as described above, with
>+      reference to a new database, and re-run <command>checksetup.pl</command>
>+      to populate it. That's all.</para>

Does it not result in a file called localconfig.foo?

>+
>+    <para>Now you have to configure the web server to pass this environment
>+      variable when accessed via an alternate URL, such as virtual host for
>+      instance:
>+<programlisting>
>+&lt;VirtualHost 212.85.153.228:80&gt;
>+    ServerName foo.bar.baz
>+    SetEnv PROJECT foo
>+    Alias /bugzilla /var/www/bugzilla
>+&lt;/VirtualHost&gt;
>+</programlisting>
>+    </para>
>+
>+    <para>Don't forget to also export this variable before accessing Bugzilla
>+       by other means, such as cron tasks for instance.</para> 
>+  </section>
> 
>   <section id="os-specific">
>     <title>OS-Specific Installation Notes</title>

Comment 33

[guillomovitch]

>What's a "Bug Base"?
A database used by bugzilla to store its data.

>Does it not result in a file called localconfig.foo?
Right, I forgot to change it.

Comment 34

[guillomovitch]

>>What's a "Bug Base"?
>A database used by bugzilla to store its data.
As this is advertized in latest official release, it would help to integrate this patch. However, I won't be able to provide a corrected version if you can't give me an official wording for thise one.

Comment 35

[Marc Schumann [:Wurblzap]]

(In reply to comment #34)
> >>What's a "Bug Base"?
> >A database used by bugzilla to store its data.
> As this is advertized in latest official release, it would help to integrate
> this patch. However, I won't be able to provide a corrected version if you
> can't give me an official wording for thise one.

I'd try "Bugzilla database"...

Don't reopen, please -- reopening refers to the code. Docs patches are handled by the "documentation?" flag.

Comment 36

[guillomovitch]

Attachment: Updated patch with minor corrections

Comment 37

[Colin Ogilvie [:cso]]

Comment on attachment 219777 [details] [diff] [review]
Updated patch with minor corrections

>Index: bugzilla/docs/xml/installation.xml
>+  <section>
>+    <title>Multiple Bugzilla database with a single installation</title>

databases

>+    <para>The previous instructions refered to a standard installation, with
>+      one unique Bugzilla database. However, you may want to host several
>+      distinct bases, without having to install Bugzilla several times.

"serveral distinct installations" would be better, I suspect.

>+      This is possible by using the PROJECT environment variable. When accessed,
>+      Bugzilla checks for the existence of this variable, and if present, uses
>+      its value to check for an alternative configuration file named
>+      <filename>localconfig.&lt;PROJECT&gt;</filename> in the same location as
>+      the default one (<filename>localconfig</filename>). It also checks for
>+      customized templates in a directory named
>+      <filename>&lt;PROJECT&gt;</filename> in the same location as the default
>+      one (<filename>default</filename>).</para> 

My mind expects to see the paths to the files after the "same location as the default one" comments.

Comment 38

[guillomovitch]

Attachment: Corrected patch

Comment 39

[Colin Ogilvie [:cso]]

Comment on attachment 222914 [details] [diff] [review]
Corrected patch

>Index: docs/xml/installation.xml
>+  <section>
>+    <title>Multiple Bugzilla databases with a single installation</title>
>+
>+    <para>The previous instructions refered to a standard installation, with
>+      one unique Bugzilla database. However, you may want to host several
>+      distinct bases, without several distinct installations.

Must have missed this in previous reviews - the "bases" doesnt' make much sense to me. From what I can tell, you want something like "However, you may want to host several distinct installations, without having several copies of the code"

>+ This is
>+      possible by using the PROJECT environment variable. When accessed,
>+      Bugzilla checks for the existence of this variable, and if present, uses
>+      its value to check for an alternative configuration file named
>+      <filename>localconfig.&lt;PROJECT&gt;</filename> in the same location as
>+      the default one (<filename>localconfig</filename>). It also checks for
>+      customized templates in a directory named
>+      <filename>&lt;PROJECT&gt;</filename> in the same location as the default
>+      one (<filename>template/default</filename>).</para> 

While I think I see what you are saying, as I understand it (and the code reads) the <PROJECT> directory should be in template/ - so it's template/<PROJECT> along side template/default... the path bit is slightly confusing to me in the documentation.

>+    <para>To set up an alternate base, just export PROJECT=foo before running
>+      <command>checksetup.pl</command> for the first time. It will result in a
>+      file called <filename>localconfig.foo</filename> instead of
>+      <filename>localconfig</filename>. Edit this file as described above, with
>+      reference to a new database, and re-run <command>checksetup.pl</command>
>+      to populate it. That's all.</para>

base -> installation

Comment 40

[guillomovitch]

You keep rejecting the doc patch for wording reasons only, this is becoming a bit frustrating... My own interest is largely reduced, now that the code itself has been merged, and I'm not a native speaker. Please, use the patch as it is now, and finish the work directly.

Comment 41

[Colin Ogilvie [:cso]]

Attachment: Patch fixing my review comments

Fixes my review comments from previous patches.

Comment 42

[Vlad Dascalu]

Comment on attachment 229096 [details] [diff] [review]
Patch fixing my review comments

Some people don't use Apache by default; the example for the vhost is for Apache, but I don't see Apache mentioned anywhere near the example.

In Lighttpd for example vhosts are configured completely in a different way.

Please mention that the example is for Apache before commiting.

Nice work :)

Comment 43

[Colin Ogilvie [:cso]]

Trunk:

Checking in installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.123; previous revision: 1.122
done

2.22:
Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.107.2.11; previous revision: 1.107.2.10
done