Skip to content
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

Plugin Support #110

Merged
merged 3 commits into from
Feb 9, 2019
Merged

Plugin Support #110

merged 3 commits into from
Feb 9, 2019

Conversation

eternal-flame-AD
Copy link
Member

@eternal-flame-AD eternal-flame-AD commented Jan 9, 2019

This fixes #51
This fixes #69

I will implement the proposal we discussed in #51 progressively. Here's a checklist:

  • Backend Database Storage for Plugins
  • Global Configuration Item
  • Plugin base interface
  • Plugin loader infrastructure && base plugin APIs
  • Webhooker
  • Configor
  • MessageSender
  • WebUI

Before merge:

@codecov
Copy link

codecov bot commented Jan 9, 2019

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #110   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          26     27    +1     
  Lines         806    822   +16     
=====================================
+ Hits          806    822   +16
Impacted Files Coverage Δ
config/config.go 100% <ø> (ø) ⬆️
database/database.go 100% <100%> (ø) ⬆️
database/plugin.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19811da...e1fa30c. Read the comment docs.

@jmattheis jmattheis self-requested a review January 9, 2019 18:48
database/plugin.go Outdated Show resolved Hide resolved
model/pluginconf.go Outdated Show resolved Hide resolved
model/pluginconf.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #110   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          26     27    +1     
  Lines         806    822   +16     
=====================================
+ Hits          806    822   +16
Impacted Files Coverage Δ
config/config.go 100% <ø> (ø) ⬆️
database/database.go 100% <100%> (ø) ⬆️
database/plugin.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19811da...b8ff408. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jan 11, 2019

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #110    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          27     39    +12     
  Lines         837   1400   +563     
======================================
+ Hits          837   1400   +563
Impacted Files Coverage Δ
auth/authentication.go 100% <ø> (ø) ⬆️
config/config.go 100% <ø> (ø) ⬆️
plugin/compat/instance.go 100% <100%> (ø)
api/stream/stream.go 100% <100%> (ø) ⬆️
test/tmpdir.go 100% <100%> (ø)
plugin/messagehandler.go 100% <100%> (ø)
api/client.go 100% <100%> (ø) ⬆️
test/asserts.go 100% <100%> (ø) ⬆️
database/plugin.go 100% <100%> (ø)
auth/token.go 100% <100%> (ø) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06d13d2...3c419b6. Read the comment docs.

@eternal-flame-AD
Copy link
Member Author

The WebUI part is not completed yet, we can review on the go part first

Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for taking time for creating this (:.

I've some remarks and I'm not finished yet as there is a lot code (:.

api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Show resolved Hide resolved
auth/authentication.go Outdated Show resolved Hide resolved
database/plugin_test.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
plugins/holder/holder.go Outdated Show resolved Hide resolved
plugins/wrapper/wrapper.go Outdated Show resolved Hide resolved
router/router.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
auth/authentication.go Outdated Show resolved Hide resolved
@jmattheis
Copy link
Member

@eternal-flame-AD I could continue working on this PR if you want/dont have time for it.

@eternal-flame-AD
Copy link
Member Author

eternal-flame-AD commented Jan 21, 2019

It's Chinese Spring Festival so actually I am on a vacation^_^ That's OK if you want to finish it :)

@jmattheis
Copy link
Member

Alright, didn't know that, then I'll wait till you're back (:.

@eternal-flame-AD
Copy link
Member Author

Thank you very much for your patience and wonderful reviewing^_^. I will come back to normal productivity shortly.

@eternal-flame-AD
Copy link
Member Author

eternal-flame-AD commented Jan 25, 2019

Up till now, I think we have a functional API and WebUI. There are several things on the TODO list:

  • Tests
  • More friendly WebUI
  • config schema verification

I think we can first review on past unresolved discussions before completing these finals

@jmattheis
Copy link
Member

Do you know why travis ci isn't building this PR?

From what I see only #110 (comment) is left (while doing it, I'll surely have some remarks again as the PR is pretty big).

@eternal-flame-AD
Copy link
Member Author

I have no idea either:(
Seems that one of the push failed to trigger a build at Travis while GitHub was waiting forever for the results on that push.

Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, here is my attempt with my idea of the plugin manager diff on branch review/plugin. I've tried to add a implementation for the following comments #110 (comment) and #110 (comment).

With these changes only one mutex exists in the plugin.Manager, furthermore I've moved a lot stuff from the PluginAPI into the plugin.Manager.

Please share your thoughts about my attempt (:.

Nothing in the branch above is tested (still need to setup a dev environment on a linux server)

test/database.go Outdated Show resolved Hide resolved
api/plugin.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
router/router.go Outdated Show resolved Hide resolved
@eternal-flame-AD
Copy link
Member Author

Okay, I would squash them now. Let's wait for a few days for the documentation to be updated and I will start to write more plugins to try out this function more deeply to spot potential underlying bugs.

@jmattheis
Copy link
Member

Should I still merge this PR or let it open, I prefer to have it closed.

plugin/manager_test.go Outdated Show resolved Hide resolved
Fixed database migration
Added a plugin system based on the go plugin package
@eternal-flame-AD eternal-flame-AD changed the title [WIP] Plugin Support Plugin Support Feb 3, 2019
@eternal-flame-AD
Copy link
Member Author

Okay I have squashed the commits. You can close this if you want to but I would recommend waiting for a few days for some new insights, since it is not ready to go to release because the documantation is not ready yet.

@jmattheis
Copy link
Member

Oke, then I'll wait. In the meantime I've create https://github.com/gotify/plugins for coordinating plugin creation. Feel free to create issues for your plugin ideas.

@eternal-flame-AD
Copy link
Member Author

Maybe take a look at https://github.com/eternal-flame-AD/gotify-netlify, I have set up a working building envrionment. Though I cannot get the local build environment exactly the same as what it is on travis, so I would get an error loading the plugin built on travis environment with a local built gotify server, but I think the problem will go away after the first official release with plugin is out.

@jmattheis
Copy link
Member

Hopefully this will work (:, otherwise plugins will not be very useful, or rather only useful for users with knowledge on building go programs.

@jmattheis
Copy link
Member

jmattheis commented Feb 4, 2019

@eternal-flame-AD Do you by chance have experience with https://github.com/yuin/gopher-lua (or some similar scripting language go thingy) it would be great to have an alternative to go plugin system, I think the plugin code is good encapsulated so it should be fairly easy to support both (If we do this, it should be done in a separate PR).

@eternal-flame-AD
Copy link
Member Author

I am not very familiar with Lua but I did consider this. I would look at this later.

First impression is that the Lua plugin has to be all in a single file in .lua extension, and no external library can be imported, so it might be somewhat limited in ability(instead of complete control of the environment as is the case of a go plugin). The good aspect is you will never have compatibility problems.

@eternal-flame-AD
Copy link
Member Author

Okay I think this is ready. You can merge this together with the documentation PR.

@jmattheis
Copy link
Member

Yeah, will do it after some final testing which I'll probably do on the weekend.

@eternal-flame-AD
Copy link
Member Author

Okay, do you want me to squash the commits again or the current state is OK?

@jmattheis
Copy link
Member

The current state is ok.

Could you have a look at https://github.com/gotify/build? I've added docker images for building gotify, this should unify the build environment and make it possible to locally build gotify/server and still use prebuild plugins and vice versa. I've updated https://github.com/gotify/plugin-template.

@eternal-flame-AD
Copy link
Member Author

eternal-flame-AD commented Feb 9, 2019

Thank you for your continuous concentration on this PR:) I think building on docker is really a good idea.

Okay, can you merge this into master or review/plugin so we can sort out the whole workflow?

As for gotify/build I think we should not specify the command for building plugins, instead tell the builder to refer to the documentation(see my documentation PR) or the template on deploying plugins, or a builder who only read the documentation of gotify/build might build the plugin without overlaying the corresponding version of server go.mod

As for the template, it will only be possible to build plugins against the master branch of server. I think we should support building against a tag, or even a commit. You can see the .travis.yml on my netlify repo for an example implementation.

Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this.

}

// EnablePlugin enables a plugin.
// swagger:operation POST /plugin/:id/enable plugin enablePlugin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:id has to be {id}. Otherwise requests can't be made from the swagger-ui.

@jmattheis
Copy link
Member

Yeah right, having a single reference how it building should be done is better.

@jmattheis jmattheis merged commit 23442bf into gotify:master Feb 9, 2019
@jmattheis jmattheis mentioned this pull request Feb 9, 2019
@eternal-flame-AD
Copy link
Member Author

Yep.

Feel free to reach to me if you want me to collaborate on the build environment or the template:)

@jmattheis jmattheis mentioned this pull request Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow updating app name and description through API/UI Pluggable extensions
3 participants