Mattermost-server: Plugin framework: Show that a plugin requires a certain Mattermost config setting

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

Add a way for plugins to specify they need the server config in a certain state. For example a plugin may need LDAP enabled if the plugin extends LDAP in some way.

An example use case is plugins overriding usernames/profile pictures, but to do that they need the Mattermost config settings to allow that to be set.

---

You can find more information about plugins in our developer documentation: https://developers.mattermost.com/extend/plugins/

If you have any questions, let us know!

As an administrator, I would expect to be informed in (at least) the following two ways:

• In the server logs (System Console > Logs)
• In the Plugins > Management page when activating the plugin

For 2), if it's not simple to add the appropriate message in the UI (e.g. if this needs additional UI design), then we could show a generic message like in the attached case and point them to the server logs for details.

[ltenfield]

I would like to grab this one... if it's still up for grabs :smile:

[jwilander]

Go for it, thanks @ltenfield !

[g3rv4]

I'd give this a try if nobody is working on it

[g3rv4]

Can you please check if my assumptions make sense?

1. Plugins should fail installation if the instance doesn't match their requirements. Write details to the log.
2. If a plugin is installed successfully and then the system configuration changes so that requirements are no longer met, activation mast fail. Write details to the log.
3. If a plugin is activated successfully and then the system configuration changes so that requirements are no longer met, it should be deactivated automatically. Write details to the log.
4. If a plugin is updated and it includes requirements that are not met by the system it should be disabled and an error logged.

[jwilander]

I'll defer to @lieut-data and the toolkit team here

[lieut-data]

@g3rv4 and I synced up offline:

• We're going to try to implement this as a pluginhelper extension (requiring a tiny bit of Go code to activate), so that plugins can start using this functionality without waiting for a server upgrade.
• Will leverage the existing config merge functionality + json serialization compare to quickly determine if the existing config has the desired settings without solving the "recursive struct walk" problem. This means slightly less clear error messages, but otherwise full functionality.
• Plugin will fail to start OnActivate with an appropriate error message.

There's technically the possibility that the configuration changes underneath the plugin, but we may or may not address that as part of these changes.

[ali-farooq0]

Opening it up to the community again due to inactivity on the PRs as it requires a little bit of more work to finish it off.

cc @hanzei

[isacikgoz]

I am interested to work on this one, if it is still available :)

[jwilander]

All yours @isacikgoz :)

[isacikgoz]

I think this could be handled in the plugin side only. Wrt @lieut-data's ideas, a simple method can be added:

// checkRequiredServerConfiguration checks if the server is configured according to
// plugin requirements.
func (p *Plugin) checkRequiredServerConfiguration(req *model.Config) (bool, error) {

    cfg := p.API.GetConfig()

    mc, err := utils.Merge(req, cfg, nil)
    if err != nil {
        return false, errors.Wrap(err, "could not merge configurations")
    }

    mergedCfg := mc.(model.Config)
    if mergedCfg.ToJson() != cfg.ToJson() {
        return false, nil
    }

    return true, nil
}

and this function can be called in pluginsOnActivate() method as:

if ok, err := p.checkRequiredServerConfiguration(req); err != nil {
    return errors.Wrap(err, "could not check required server configuration")
} else if !ok {
    return errors.New("server configuration is not compatible")
}

This solution conforms to informing Administrator with following :

• [x] In the server logs (System Console > Logs)
• [x] In the Plugins > Management page when activating the plugin

The remaining work is to decide where to define the required configuration :)

Since it is not a plugin configuration, it shouldn't defined in any JSON file that means no problem if it is hardcoded. My idea is to define it in checkRequiredServerConfiguration or onActivate method.

[lieut-data]

@isacikgoz, I think your analysis makes sense! One note:

Since it is not a plugin configuration, it shouldn't defined in any JSON file that means no problem if it is hardcoded. My idea is to define it in checkRequiredServerConfiguration or onActivate method.

I think this belongs in the plugin manifest (i.e. plugin.json). The primary benefit will be the ability to introspect these requirements programmatically without having to parse the source code. This is especially relevant to the plugin marketplace in v5.16, where, for example, we detect the minimum server version defined in plugin.json and tailor the installable plugins for a given server version accordingly. It would be ideal to know about these configuration requirements up front so as to potentially pre-communicate to the system administrator.

[isacikgoz]

@lieut-data, I agree that it would be better to define required configuration in plugin.json. Somehow I ended up forgetting that these values (i.e required configuration) may be accessible from outside (e.g. marketplace).

I will add this into my PR, thank you very much!