search

mattermost / mattermost-plugin-starter-template

Build scripts and templates for writing Mattermost plugins.
https://developers.mattermost.com/extend/plugins/
Apache License 2.0
133 stars 121 forks source link

configuration: ignore an empty configuration struct in setConfiguration #21

Closed lieut-data closed 5 years ago

lieut-data commented 6 years ago

If the plugin leaves the configuration struct empty, go will optimize away allocations of the zero-width struct, failing the setConfiguration check that prevents a user from introducing race conditions.

The demo plugin has more extensive unit tests around configuration, but it also has a non-empty struct as such. I'm inclined /not/ to test this case in the sample plugin, because this is meant as a "copy and paste" way to get started, and if you add a configuration, the test would fail.

Fixes #17

lieut-data commented 5 years ago

@hanzei, yeah it's tricky. I mean we could throw a test into the server, but we're not shipping the code under test /with/ the server: it's here (and in a slightly different way, in the demo plugin).

And in a real-life situation, if the plugin didn't define a configuration, it would probably make the most sense to just delete configuration.go altogether.

hanzei commented 5 years ago

@hanzei, yeah it's tricky. I mean we could throw a test into the server, but we're not shipping the code under test /with/ the server: it's here (and in a slightly different way, in the demo plugin).

@lieut-data I was more thinking about a set of "common" plugins which get tested on every build on the server. This would make us more aware when we break things.

zeroimpl commented 5 years ago

I think this should be done on mattermost-plugin-demo too. One of the first things I did was remove all fields from the struct in that file, which led it to fatal. I didn't delete the file since I planned to add new configuration fields later.

hanzei commented 5 years ago

@zeroimpl I agree. We should fix this both plugins. @lieut-data @DSchalla what do think?

lieut-data commented 5 years ago

@hanzei, @zeroimpl -- 100% agree. Maybe we sync up the sample and demo plugins once @DSchalla's delve changes go in?

DSchalla commented 5 years ago

Yeah that sounds good! Since the delve changes are still in the works, personally I'd be good with adding the change already now to the sample, unless there are multiple changes already queued up - in that case we can also wait to sync them up in one sweep.