Avoid reconfiguring job on when irrelevant server config values change

[mickmister]

Summary

This PR makes it so the plugin reconfigures itself on OnConfigurationChange only when changes occur in settings that the plugin depends on. This PR is more of a POC to discuss how this should work.

Ticket Link

Fixes https://github.com/mattermost/mattermost-plugin-legal-hold/issues/45

Related to https://github.com/mattermost/mattermost-plugin-legal-hold/pull/46

[fmartingr]

I like the idea (I didn't knew about this OnConfigurationChange behavior) but I would love it this would be more bundled in the Plugin API to avoid boilerplate, since most OnConfigurationChange functions looks the same in all plugins.

Does it make sense to have something like:

type PluginAPI interface {
    // ...
    OnConfigurationChange() error
    SetReconfigureMatcher(ConfigMatcherFunc)
        Reconfigure(old, new Config) error
}

type ConfigMatcherFunc (old, new Config) bool

func DefaultConfigMatcher(_, _ Config) bool { // maybe (bool, error) ?
    return true
}

func (p *PluginAPI) Reconfigure(old, new Config) error {
    // only called if ConfigMatcherFunc returns true
}

Maybe even moving the OnConfigurationChange part to an internal part of the Plugin API and use the SetReconfigure... as the condition for that function to be called.

Just an idea. What do you think?

[mickmister]

@fmartingr One issue here is that the server is currently not explicitly keeping track of what config values changed in a given config-mutating API request. On a given plugin settings page, the API request contains the entire server config object.

The plugin may want to know about changes to config values outside of its own plugin settings as well. That's currently the case with all the file settings stuff this plugin uses. I don't think there's a silver bullet here, and I think the current situation allows plugins to handle changes that they care about.

The more hand-holdy it can get, the more corner cases are missed, and devs would likely fallback to the most permissive situation, which is what we currently have. I like your direction of improving this though. Maybe something to bring up to other plugin devs at the company for some feedback on these ideas.

[mickmister]

@fmartingr I think focusing on what "recreating the job" means is more practical than what this PR is trying to do. The process of "reconfiguring" or "recreating" the job should ideally be idempotent and have minimal impact on performance and data integrity. We should also think about how this process affects HA setups.

Having the plugin continue to reconfigure itself no matter what config changes happen should be the strategy here I think. I'm closing this PR and replacing the task of https://github.com/mattermost/mattermost-plugin-legal-hold/issues/45 to be verifying the behavior of reconfiguring the job on config change, and ensure that it's resilient to being called potentially often.