Add boilerplate for api, commands, kvstore access and job management #212

BenCookie95 · 2024-11-08T21:54:51Z

Summary

Problem:
The starter template is too bare bones to kickstart most plugin efforts, requiring a lot of copy and paste from “known good” plugins to achieve a stable starting point.

Justification:
DE doesn’t use the starter template all that often anymore, spending extra time bootstrapping a plugin with all the necessary plumbing. This also leads to unnecessary divergences between plugins, complicating our developer integration strategy and story.

I added some skeleton code for creating an api and adding new slash commands. I separated the slash commands and KVStore access into their own packages for ease of use. I also added a simple mock for the command package to show how these separate packages can be easily tested.

I noticed we used this Job Manager code across a few different plugins and I thought it would be very useful to bring into the skeleton.

Ticket Link

https://mattermost.atlassian.net/browse/MM-61468
https://mattermost.atlassian.net/browse/MM-60911
https://mattermost.atlassian.net/browse/MM-60910
https://mattermost.atlassian.net/browse/MM-60909

hanzei

I really appreciate the work you are doing for the starter template. This is a great way for community members to build plugins more easily.

I wonder if we should make the boilerplate code opt-in. Something like a cli tool that generates a fresh repo with all the code that was specified. go templates could drive such a CLI tool.

BenCookie95 · 2024-11-11T14:06:23Z

I really appreciate the work you are doing for the starter template. This is a great way for community members to build plugins more easily.

I wonder if we should make the boilerplate code opt-in. Something like a cli tool that generates a fresh repo with all the code that was specified. go templates could drive such a CLI tool.

Go templates is a great suggestion and maybe something we can use in future! I know that too much boilerplate can be annoying but I think this current implementation is easy to manage because most stuff is separated into separate packages and it's just a case of deleting the package and the initialization logic in plugin.go.

lieut-data

Thanks, @BenCookie95! A few of my inline comments are more exploratory and aren't blocking, but a few high level thoughts I'd love to work through:

I'm not a big fan of packages for packages sake, e.g. configuration. Similarly, command, and especially the resulting interface just makes navigating the code all that much harder. Could we hoist some of the common code into the public package and make this simple enough to have all the code live in the main package?
The job manager feels like it comes with a lot of baggage that I'd love to avoid adding to the template. Could we illustrate running an HA-aware job using the existing cluster package instead?

lieut-data · 2024-11-13T21:07:24Z

server/api.go

+	router.ServeHTTP(w, r)
+}
+
+func (p *Plugin) MattermostAuthorizationRequired(next http.Handler) http.Handler {


It would be awesome to hoist this into the public package.

Yeah that makes sense

server/api.go

server/command/command.go

lieut-data · 2024-11-13T21:10:41Z

server/command/command.go

+const helloCommandTrigger = "hello"
+
+// Register all your slash commands in the NewCommandHandler function.
+func NewCommandHandler(client *pluginapi.Client) Command {


One day, it would be awesome to hoist this into the public package and just register callbacks for the slash commands to be handled.

In that future, I wonder if we would need a discrete package at all and could just do this all in command.go?

lieut-data · 2024-11-13T21:11:28Z

server/command/command.go

+		AutoComplete:     true,
+		AutoCompleteDesc: "Say hello to someone",
+		AutoCompleteHint: "<username>",


I wonder if we should illustrate the advanced autocomplete functionality?

What is that advanced functionality? I'm not aware, is there something I can read?

Take a peek at https://github.com/mattermost/mattermost-plugin-playbooks/blob/6c50600fb6151ea1abfed82961e980131b0ca2ab/server/command/command.go#L61 (ironically in an unnecessary command package itself ;P)

server/jobs/jobs_util.go

server/jobs/startertemplate_job.go

server/plugin.go

lieut-data · 2024-11-13T21:27:25Z

I know that too much boilerplate can be annoying but I think this current implementation is easy to manage because most stuff is separated into separate packages and it's just a case of deleting the package and the initialization logic in plugin.go.

👍 to just having code vs. generating. That being said, as noted above, I'm not a huge fan of packages achieve this, so hopefully we can find a middle ground where it's still easy to remove but doesn't come with lots of cognitive overhead in the way the plugin is structured.

BenCookie95 · 2024-11-13T21:56:12Z

I'm not a big fan of packages for packages sake, e.g. configuration. Similarly, command, and especially the resulting interface just makes navigating the code all that much harder. Could we hoist some of the common code into the public package and make this simple enough to have all the code live in the main package?

My main thought process of creating separate packages from the start was to discourage the overuse of the main package. I've read through a bunch of plugins and it gets very hard to navigate or figure out where anything is when it lives in the main package. You mentioned that you aren't a fan of using packages due to the cognitive overhead in the way the plugin is structured. Am I alone in wanting more packages to achieve this? I personally find it much easier to read and navigate.

I agree that I could move the config back into the main package because it doesn't serve much use as it's own package (probably went overboard with that one). Command in it's current state made sense to be in a separate package because there is a fair amount of boilerplate to work through. I'll take a look at hoisting some of that to the public package and then it would probably make sense to move it back to main?

The job manager feels like it comes with a lot of baggage that I'd love to avoid adding to the template. Could we illustrate running an HA-aware job using the existing cluster package instead?

Yeah, my initial thoughts were from a DE perspective because this was reused in a few plugins but it's overkill for a contributor coming in. I'll simplify it. Demo plugin already has a simple use of the cluster package that I will re-use

BenCookie95 · 2024-11-13T22:40:54Z

Resolved most of the comments. Removed the config package and cleared out the overly complicated Job stuff

lieut-data · 2024-11-15T18:53:45Z

My main thought process of creating separate packages from the start was to discourage the overuse of the main package. I've read through a bunch of plugins and it gets very hard to navigate or figure out where anything is when it lives in the main package. You mentioned that you aren't a fan of using packages due to the cognitive overhead in the way the plugin is structured. Am I alone in wanting more packages to achieve this? I personally find it much easier to read and navigate.

@BenCookie95, for context, I recently went through an exercise in MS Teams collapsing many of the packages back into the main package. Most of the secondary packages were tightly coupled with the Plugin and required lots of interface juggling when more complex business logic was required. I was able to eliminate maybe 25% of the code overhead and make the code easier to navigate without unhelpful interfaces. I'm curious how you might find reading https://github.com/mattermost/mattermost-plugin-msteams/tree/main/server (not that it's perfect by any stretch!)

On the "when" side for new packages, I was chatting with @crspeller and wrote down a few shared thoughts:

Fewer packages is better: default to the main package unless there's good reason for a new package.
Coupling implies same package: don't jump through hoops to break apart code that's naturally coupled.
New package for a new interface: a classic example is the sqlstore with layers for monitoring performance, caching and mocking.
New package for upstream integration: a discrete client package for interfacing with a 3rd party is often a great place to break out into a new package

Curious as to your thoughts?

lieut-data

Thanks @BenCookie95! One comment about the commented code, but otherwise non-blocking.

lieut-data · 2024-11-15T18:54:53Z

server/command/command.go

+		AutoComplete:     true,
+		AutoCompleteDesc: "Say hello to someone",
+		AutoCompleteHint: "<username>",


Take a peek at https://github.com/mattermost/mattermost-plugin-playbooks/blob/6c50600fb6151ea1abfed82961e980131b0ca2ab/server/command/command.go#L61 (ironically in an unnecessary command package itself ;P)

server/configuration.go

server/plugin.go

server/job.go

server/api.go

BenCookie95 · 2024-11-22T21:09:17Z

My main thought process of creating separate packages from the start was to discourage the overuse of the main package. I've read through a bunch of plugins and it gets very hard to navigate or figure out where anything is when it lives in the main package. You mentioned that you aren't a fan of using packages due to the cognitive overhead in the way the plugin is structured. Am I alone in wanting more packages to achieve this? I personally find it much easier to read and navigate.

@BenCookie95, for context, I recently went through an exercise in MS Teams collapsing many of the packages back into the main package. Most of the secondary packages were tightly coupled with the Plugin and required lots of interface juggling when more complex business logic was required. I was able to eliminate maybe 25% of the code overhead and make the code easier to navigate without unhelpful interfaces. I'm curious how you might find reading https://github.com/mattermost/mattermost-plugin-msteams/tree/main/server (not that it's perfect by any stretch!)

On the "when" side for new packages, I was chatting with @crspeller and wrote down a few shared thoughts:

Fewer packages is better: default to the main package unless there's good reason for a new package.

Coupling implies same package: don't jump through hoops to break apart code that's naturally coupled.

New package for a new interface: a classic example is the sqlstore with layers for monitoring performance, caching and mocking.

New package for upstream integration: a discrete client package for interfacing with a 3rd party is often a great place to break out into a new package

Curious as to your thoughts?

I'm happy with that criteria you laid out and I think I'll add it to the README of this plugin so contributors have some guidance. Thanks!

required lots of interface juggling when more complex business logic was required

I can see how that can be painful if overused

wiggin77 · 2024-12-10T02:57:55Z

README.md

+
+This is a central place for you to access the KVStore methods that are available in the `pluginapi.Client`. The package contains an interface for you to define your methods that will wrap the KVStore methods. An instance of the KVStore is created in the `OnActivate` hook.
+
+#### Jobs package


Not a package anymore.

wiggin77

LGTM 👍 One change needed to readme.

BenCookie95 added 2 commits November 8, 2024 16:48

add boilerplate for api, commands and jobs

3fd287d

tidy

7b20019

hanzei reviewed Nov 11, 2024

View reviewed changes

BenCookie95 added 3 commits November 13, 2024 13:02

add some instructions to the readme about how to use the new boilerplate

a8b8f35

wording

3da5172

fix error message

4e3f0ef

BenCookie95 changed the title ~~WIP: Add boilerplate for api, commands and jobs~~ Add boilerplate for api, commands, kvstore access and job management Nov 13, 2024

BenCookie95 requested review from lieut-data and wiggin77 November 13, 2024 18:30

lieut-data requested changes Nov 13, 2024

View reviewed changes

BenCookie95 added 3 commits November 13, 2024 17:21

use cluster package for creating a job and remove other job clutter

03035b7

tidy, move configuration and remove oninstall

51b7bdd

adding log for api failure and tidy up error type

83b964e

BenCookie95 requested a review from lieut-data November 13, 2024 22:40

lieut-data approved these changes Nov 15, 2024

View reviewed changes

adjusting for comments

639fbcc

hanzei added the 2: Dev Review Requires review by a core committer label Dec 5, 2024

wiggin77 reviewed Dec 10, 2024

View reviewed changes

wiggin77 approved these changes Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add boilerplate for api, commands, kvstore access and job management #212

Add boilerplate for api, commands, kvstore access and job management #212

BenCookie95 commented Nov 8, 2024 •

edited

Loading

hanzei left a comment

BenCookie95 commented Nov 11, 2024 •

edited

Loading

lieut-data left a comment

lieut-data Nov 13, 2024

BenCookie95 Nov 13, 2024

lieut-data Nov 13, 2024

lieut-data Nov 13, 2024

BenCookie95 Nov 13, 2024

lieut-data Nov 15, 2024

lieut-data commented Nov 13, 2024

BenCookie95 commented Nov 13, 2024 •

edited

Loading

BenCookie95 commented Nov 13, 2024

lieut-data commented Nov 15, 2024

lieut-data left a comment

lieut-data Nov 15, 2024

BenCookie95 commented Nov 22, 2024

wiggin77 Dec 10, 2024

wiggin77 left a comment


		This is a central place for you to access the KVStore methods that are available in the `pluginapi.Client`. The package contains an interface for you to define your methods that will wrap the KVStore methods. An instance of the KVStore is created in the `OnActivate` hook.

		#### Jobs package

Add boilerplate for api, commands, kvstore access and job management #212

Are you sure you want to change the base?

Add boilerplate for api, commands, kvstore access and job management #212

Conversation

BenCookie95 commented Nov 8, 2024 • edited Loading

Summary

Ticket Link

hanzei left a comment

Choose a reason for hiding this comment

BenCookie95 commented Nov 11, 2024 • edited Loading

lieut-data left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lieut-data commented Nov 13, 2024

BenCookie95 commented Nov 13, 2024 • edited Loading

BenCookie95 commented Nov 13, 2024

lieut-data commented Nov 15, 2024

lieut-data left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenCookie95 commented Nov 22, 2024

Choose a reason for hiding this comment

wiggin77 left a comment

Choose a reason for hiding this comment

BenCookie95 commented Nov 8, 2024 •

edited

Loading

BenCookie95 commented Nov 11, 2024 •

edited

Loading

BenCookie95 commented Nov 13, 2024 •

edited

Loading