415278 - Make the WebService's plugin discovery method more extendable

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Max Kanat-Alexander]

Right now we return information about plugins like this:

(plugin_name => $version, plugin_name2 => $version2)

But what if we want to return more info about a plugin in the future than just its version? We need to instead return things like this:

plugin_name => { version => $version }

That's much more extendable.

Comment 1

[Colin Ogilvie [:cso]]

Will work on this shortly.

Comment 2

[Colin Ogilvie [:cso]]

Attachment: Patch v1

I've changed the extension file from version.pl to be info.pl which then returns a hash. Think I've tested everything as well.

Will upload my test info.pl shortly.

Comment 3

[Colin Ogilvie [:cso]]

Attachment: info.pl

Comment 4

[Max Kanat-Alexander]

Comment on attachment 312520 [details] [diff] [review]
Patch v1

>+        $enabled{$extname} = do("$extension/info.pl");

  You'll also have to fix the example plugin.

>@@ -125,9 +125,11 @@
>+Every extension must have a file in its root called F<info.pl>.
>+This file should return a hash when called with C<do>.
>+The hash should contain a 'version' element with the current version of the
>+extension. Plugin authors can also add any extra infomration to this has if
>+required.

  We should specify this a bit more. We should define the fields that they must return (version), and specify that any other fields should start with x_, which is a namespace that we promise not to use for Bugzilla itself.

> 
> =head1 SUBROUTINES
> 
>Index: Bugzilla/WebService/Bugzilla.pm
>+        $plugins->{$name} = $plugins->{$name};

  That doesn't look to be doing anything.

  Remember that every field we know about should be explicitly typed, if possible. (That is, "version" should be made a type('string').)

Comment 5

[Colin Ogilvie [:cso]]

(In reply to comment #4)
> (From update of attachment 312520 [details] [diff] [review])
> >+        $enabled{$extname} = do("$extension/info.pl");
> 
>   You'll also have to fix the example plugin.

That was the following file I added.

>   Remember that every field we know about should be explicitly typed, if
> possible. (That is, "version" should be made a type('string').)

How can you explicitly type something, if it's free-form -- we don't know whether an author will want to return a string for example or an integer? 

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> That was the following file I added.

  Yeah, but a patch that also removes version.pl would be good.

> How can you explicitly type something, if it's free-form -- we don't know
> whether an author will want to return a string for example or an integer? 

  Just type the version field.

Comment 7

[Colin Ogilvie [:cso]]

Attachment: Patch v2

Fixes mkanat's review comments... I have no idea how to remove a file or show a new file in a `cvs diff` though.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> Fixes mkanat's review comments... I have no idea how to remove a file or show a
> new file in a `cvs diff` though.

  You can use cvsdo for that, if you're in a checkout without checkin access.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 321390 [details] [diff] [review]
Patch v2

This is good, except I want to use the word "Extensions" instead of "Plugins" now, which means also changing the name of the function in the WebService.

Comment 10

[Colin Ogilvie [:cso]]

Attachment: Patch v3

Hmm, cvsdo is useful...

Patch that changes various bits to Extensions rather than plugins.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 321441 [details] [diff] [review]
Patch v3

>+++ extensions/example/info.pl	17 May 2008 23:05:52 -0000
>+# Contributor(s): Max Kanat-Alexander <[e-mail]>

  You should add yourself.

>+# This script does some code to return a version number. However,
>+# most plugins will probably just want to return a raw string.
>+# To do that, the only contents of the file should be the string
>+# on a single line, like:
>+#
>+# '1.2.3';

  That comment should be updated.

>+no warnings qw(void); # Avoid "useless use of a constant in void context"

  I think that's not necessary anymore?

>Index: Bugzilla/WebService/Bugzilla.pm
> [snip]
>+A hash with a single item, C<extesions>. This points to a hash. I<That> hash
>+contains the names of extensions as keys, and information about the extension
>+as values.

  That needs to be more specific--you need to say that the info hash contains a "version" key, however you can best get that across.

>Index: contrib/bz_webservice_demo.pl
>+If specified on the command line, the script returns the information about the
>+extensions that are installed for the Webservices.

  It's just extensions that are installed, not 'for the Webservices.'


  Everything else looks good!

Comment 12

[Colin Ogilvie [:cso]]

Attachment: Patch v4

Hopefully this time...

Comment 13

[Max Kanat-Alexander]

Comment on attachment 321470 [details] [diff] [review]
Patch v4

Looks good!

Comment 14

[Colin Ogilvie [:cso]]

Asking for approval on trunk/3.2 as I'm not sure if it's branched yet... someone let me know before I check it in :)

Comment 15

[Frédéric Buclin]

We didn't branch yet. So a+ for immediate checkin.

Comment 16

[Colin Ogilvie [:cso]]

Checking in Bugzilla/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v  <--  Hook.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/WebService/Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bugzilla.pm,v  <--  Bugzi                                                                             lla.pm
new revision: 1.7; previous revision: 1.6
done
Checking in contrib/bz_webservice_demo.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bz_webservice_demo.pl,v  <--  bz_webs                                                                             ervice_demo.pl
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/extensions/example/info.pl,v
done
Checking in extensions/example/info.pl;
/cvsroot/mozilla/webtools/bugzilla/extensions/example/info.pl,v  <--  info.pl
initial revision: 1.1
done
Removing extensions/example/version.pl;
/cvsroot/mozilla/webtools/bugzilla/extensions/example/version.pl,v  <--  version                                                                             .pl
new revision: delete; previous revision: 1.1
done

Comment 17

[Frédéric Buclin]

test script: webservice_bugzilla.t