Proposal: Support avatars plugin.

[dmitshur]

Per https://github.com/andygrunwald/go-gerrit#what-about-adding-code-to-support-the-rest-api-of-an-optional-plugin, making an issue first.

There appears to be some sort of "avatars" plugin or extension, which as I understand actually has multiple different implementations (e.g., avatars-gravatar, avatars-external, possibly others).

There's a dedicated has_avatars bool for it in PluginConfigInfo entity.

It seems to be popular, enough to be on all 3 of the Gerrit servers I tried:

$ curl -s 'https://gerrit-review.googlesource.com/config/server/info' | grep has_avatars
    "has_avatars": true,
$ curl -s 'https://go-review.googlesource.com/config/server/info' | grep has_avatars
    "has_avatars": true,
$ curl -s 'https://upspin-review.googlesource.com/config/server/info' | grep has_avatars
    "has_avatars": true,

As far as I can tell (but I haven't been able to verify this with full certainty), when that plugin/extension is on, the AccountInfo entity contains an additional undocumented avatars field:

$ curl 'https://gerrit-review.googlesource.com/accounts/1010008'
)]}'
{
  "_account_id": 1010008,
  "name": "Example Example",
  "email": "example@example.com",
  "avatars": [
    {
      "url": "https://lh3.googleusercontent.com/-qjpKeQViQbI/AAAAAAAAAAI/AAAAAAAAAAA/_CVwpoi1Y8Y/s26-p/photo.jpg",
      "height": 26
    },
    {
      "url": "https://lh3.googleusercontent.com/-qjpKeQViQbI/AAAAAAAAAAI/AAAAAAAAAAA/_CVwpoi1Y8Y/s32-p/photo.jpg",
      "height": 32
    },
    {
      "url": "https://lh3.googleusercontent.com/-qjpKeQViQbI/AAAAAAAAAAI/AAAAAAAAAAA/_CVwpoi1Y8Y/s100-p/photo.jpg",
      "height": 100
    }
  ]
}

So, is it okay to send a PR that'll add support for that field?

[andygrunwald]

All three instances seems to be a Google hosted instances.

Other instances (not google ones) don't seem to have this plugin:

curl -s 'https://review.typo3.org/config/server/info' | grep has_avatars

curl -s 'https://gerrit.wikimedia.org/r/config/server/info' | grep has_avatars

curl -s 'https://review.openstack.org/config/server/info' | grep has_avatars

Of course we could check a couple of more non google instances. See https://en.wikipedia.org/wiki/Gerrit_(software)#Notable_users

I am thinking about a question like "How should we decide when to integrate a plugin and when not?". What are the criteria? Or might there be a better way of integration plugins without adding them to the complete native Gerrit code base here? So that we don't need to think about this question about adding them or not at all, because it don't matter?

[dmitshur]

Yes, those are good questions and we need to figure out how to answer them in order to proceed.

How should we decide when to integrate a plugin and when not? What are the criteria?

I'm not sure myself. It might be that we just have to make a decision, somehow.

If that's the case, I think you as the project owner are in the best position to make a decision.

Or might there be a better way of integration plugins without adding them to the complete native Gerrit code base here?

It would be really nice if that's the case, but I can't think of a way of making that possible at this time.

For reference, my local patch (after a bit of cleaning up) for this tries to make the Avatars field separate and documents it as only being present if the avatars plugin is enabled:

diff --git a/accounts.go b/accounts.go
index 63df455..47e9500 100644
--- a/accounts.go
+++ b/accounts.go
@@ -17,6 +17,13 @@ type AccountInfo struct {
    Name      string `json:"name,omitempty"`
    Email     string `json:"email,omitempty"`
    Username  string `json:"username,omitempty"`
+
+   // Avatars lists avatars of various sizes for the account.
+   // This field is only populated if the avatars plugin is enabled.
+   Avatars []struct {
+       URL    string `json:"url,omitempty"`
+       Height int    `json:"height,omitempty"`
+   } `json:"avatars,omitempty"`
 }

 // SSHKeyInfo entity contains information about an SSH key of a user.
diff --git a/config.go b/config.go
index e5540f2..088b0fb 100644
--- a/config.go
+++ b/config.go
@@ -143,6 +143,7 @@ type ReceiveInfo struct {

 // PluginConfigInfo entity contains information about Gerrit extensions by plugins.
 type PluginConfigInfo struct {
+   // HasAvatars reports whether an avatar provider is registered.
    HasAvatars bool `json:"has_avatars,omitempty"`
 }

I'm not aware of a way to make it better at this time, but I haven't thought very long about it yet.

[andygrunwald]

I'm not sure myself. It might be that we just have to make a decision, somehow.

True.

If that's the case, I think you as the project owner are in the best position to make a decision.

I am not the boss here. We are a team and contributing together on this. Only because I started this doesn't mean that I am the boss :) I often think that multiple opinions and a good discussion lead to a better result.

For reference, my local patch ...

I think your patch looks quite good. One point we can make: Once a popular number of Gerrit instances (and i count the google instances as popular) supports this, we can implement this. The instances I raised don't support this. Then the Avatars is empty. We should mark the fields and methods then as a plugin field (via doc?) to make it clear that this is not a native functionality here.

After some thoughts, I think we should be pragmatic here. This is the second plugin now. I don't think that 1.000 plugins will be implemented in the next time in this lib.

Ergo: Go ahaed :)