GH-9737: Allow setting min_server_version in plugin manifest #9743
Conversation
|
Would there be a sample plugin we can test the UX with? |
jasonblais
added
1: PM Review
|
Sure, here you go.com.mattermost.sample-plugin-0.0.1.tar.gz I set |
There was a problem hiding this comment.
@hanzei You're awesome, that's so fast. Just a minor comment/inquiry.
|
Work on this PR is paused until some discussion about this feature is finished. You can follow it here |
|
I saw that the discussion on pre-release regarding this PR is completed. Let me know once updates are completed and I'll review the 'work in progress' label (and if no updates are needed, let me know :) ) |
|
@jasonblais The outcome of the discussion is that a |
jasonblais
removed
Setup Old Test Server
|
|
|
Spinmint test server created at: https://i-00f27f2a0a685c0e7.test.spinmint.com Test Admin Account: Username: Test User Account: Username: Instance ID: i-00f27f2a0a685c0e7 |
There was a problem hiding this comment.
Thanks @hanzei. Functionally it works for me with your sample plugin. Just added one question on the error message itself.
As you say, the UX could be improved, but I don't think it blocks this PR - the error message asks the admin to view the error in server logs, which states plugin requires Mattermost 5.6.0: com.mattermost.sample-plugin. I suspect this is the same for other such messages.
One question: Do you know how easy or hard it would be to present the error message we output to server logs in the System Console > Plugins > Management page directly? It would save a step for the admin. If easy, I can create a ticket for it.
PS: I also created a ticket for a bug when uploading a plugin - reproduces on master so not caused by this PR. https://mattermost.atlassian.net/browse/MM-12892
jasonblais
added
2: Dev Review
jwilander
added
4: Reviews Complete
|
Spinmint test server destroyed |
@jasonblais I think this is not so easy to do. But others can estimate this better. |
amyblais
added
Docs/Needed
lindy65
added
Tests/Not Needed
jasonblais
added
Docs/Not Needed
This takes advantage of built-in version constraint checking within [Mattermost v5.6+](mattermost/mattermost#9743). I realize this won't actually enforce the version check prior to v5.6+, but thought I'd submit in case it that was acceptable.
This takes advantage of built-in version constraint checking within [Mattermost v5.6+](mattermost/mattermost#9743). I realize this won't actually enforce the version check prior to v5.6+, but thought I'd submit in case it that was acceptable.
This takes advantage of built-in version constraint checking within [Mattermost v5.6+](mattermost/mattermost#9743). I realize this won't actually enforce the version check prior to v5.6+, but thought I'd submit in case it that was acceptable.
Summary
This PR add
min_server_versionto plugin manifests. This allows to set a minimum Mattermost server version which is required for the plugin to start.Two notes on backwards compatibility: If a plugin doesn't set
min_server_versionthe sever doesn't perform a version check and just starts the plugin. This way plugins aren't force the set the option.If a plugin sets
min_server_versionand the plugins gets executed on a Mattermost server prior to v5.6 the server will just ignore the setting. This means plugins can't enforce a server version on servers prior to v5.6!UX
If an admin tries to enable a plugin, which requires an higher server version then provided, they will see the error message "This plugin failed to start. Check your system logs for errors.". This is the same message that e.g. appears when the call of
OnActivate()fails or a plugin in badly configured. Of course we could create a new error message for this or disable the "enable" button for the plugin, but this will require changes in the webapp which I'm not capable of.The here proposed changes to the server side have to be done anyway, better UX in the webapp would be nice to have.
Ticket Link
Fixes #9737
Open questions
This PR adds a new library to
Gopkg.toml. Is this fine? Is there anything more to do?The are currently no tests for
environment.go, so I didn't add one. Doing this requires a lot of work.Error messages in
(env *Environment) Activate()are appended with the plugin id. For constancy I constructed my error messages the same way. But it makes little sense to be because in the Mattermost log will also show the plugin id. I propose to remove the id from all error messages, but 2/5 and must likely to out of scope of this PR.