-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PLT-7407: Back-end plugins #7409
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add some more tests before merging.
@@ -150,7 +150,7 @@ func InitApi(full bool) { | |||
BaseRoutes.PublicFile = BaseRoutes.Root.PathPrefix("/files/{file_id:[A-Za-z0-9]+}/public").Subrouter() | |||
|
|||
BaseRoutes.Plugins = BaseRoutes.ApiRoot.PathPrefix("/plugins").Subrouter() | |||
BaseRoutes.Plugin = BaseRoutes.Plugins.PathPrefix("/{plugin_id:[A-Za-z0-9\\_\\-]+}").Subrouter() | |||
BaseRoutes.Plugin = BaseRoutes.Plugins.PathPrefix("/{plugin_id:[A-Za-z0-9\\_\\-\\.]+}").Subrouter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposing we allow dots to facilitate reverse-dns notation for plugin ids.
@@ -19,63 +19,96 @@ import ( | |||
"github.com/mattermost/mattermost-server/model" | |||
"github.com/mattermost/mattermost-server/utils" | |||
|
|||
"github.com/mattermost/mattermost-server/app/plugin" | |||
builtinplugin "github.com/mattermost/mattermost-server/app/plugin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relabeling the pre-existing stuff as "built-in" plugins. Once we port the JIRA / LDAP stuff, we can get rid of this altogether.
utils/extract.go
Outdated
@@ -61,7 +61,7 @@ func ExtractTarGz(gzipStream io.Reader, dst string) ([]string, error) { | |||
return nil, fmt.Errorf("ExtractTarGz: MkdirAll() failed: %s", err.Error()) | |||
} | |||
|
|||
outFile, err := os.Create(path) | |||
outFile, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, os.FileMode(header.Mode)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotta preserve permissions for executables.
plugin/pluginenv/environment.go
Outdated
params := mux.Vars(r) | ||
if id, ok := params["plugin_id"]; ok { | ||
h.env.mutex.Lock() | ||
defer h.env.mutex.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Concurrency. :P This lock shouldn't be too hard to optimize out, and I'm pretty positive we'll want to do that, but I'm putting it off until later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a ticket for this: https://mattermost.atlassian.net/browse/PLT-7572
Somewhat unrelated but I wonder if we should allow a system admin the ability to disable plugin uploads. So for the ultra paranoid you can only manually install plugins by copying the files to server yourself. |
Created a task to further expand the API: https://mattermost.atlassian.net/browse/PLT-7608 And made a really basic repo if anyone wants to actually see a "hello world" plugin: https://github.com/ccbrown/mattermost-plugin-demo Once we have a bit more of an API, maybe I can make it into a cooler demo. |
if len(splitPath) == 0 { | ||
return nil, model.NewAppError("UnpackAndActivatePlugin", "app.plugin.bad_path.app_error", nil, err.Error(), http.StatusBadRequest) | ||
if len(dir) == 1 && dir[0].IsDir() { | ||
tmpPluginDir = filepath.Join(tmpPluginDir, dir[0].Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this up a little to allow tarballs where files are placed directly into the root. For example, this should work:
tar -czvf mattermost-plugin-demo.tar.gz mattermost-plugin-demo plugin.yaml
}) | ||
return nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eliminating a race condition. Hoping for Go 1.10 we'll get a way to safely interrupt or time out (*os.File).Read
, simplifying all of this. (See golang/go#18507)
@coreyhulen The option to disable plugin uploads might be a good idea for our open-to-world test servers. Of course, another good idea might be to require VPN for those. :-/ |
Summary
It is finished.
Ticket Link
https://mattermost.atlassian.net/browse/PLT-7407?filter=14400
Checklist