Multi product architecture #3309

wiggin77 · 2022-07-01T19:38:41Z

Summary

This PR provides support for compiling Boards directly into the Mattermost suite server. This required a major refactoring of the mattermost-plugin package. Plugin builds and stand-alone continue to be supported.

Notable changes:

a ServicesAPI interface replaces the PluginAPI to allow for implementations coming from pluginAPI and suite server.
a new product package provides a place to register Boards as a suite product and handles life-cycle events
a new boards package replaces much of the mattermost-plugin logic, allowing this to be shared between plugin and product
Boards now uses module workspaces; run make setup-go-work or place a go.work in the repo root containing:

go 1.18

use ./mattermost-plugin
use ./server
use ../mattermost-server

go.mod should no longer use replace to import mattermost-server/v6

Ticket Link

https://community.mattermost.com/boards/workspace/qgsck6cts3fwpqwyjiupjm5cde/b4ctmm917q3rzdfi451qtuxjzcw/150d1464-6d59-4c39-810f-ee201439dbaf/cfs87fp368fbwmc45wnuzcadaqe

…tecture

…s; remove logger service

…tecture

…inAPI interfaces for all packages

…tecture

jespino

It is a big change and I haven't look into each detail here, but looks good to me.

One tiny concern is we still have everything under the mattermost-plugin directory, so... plugin code, and product code are not clearly separated in that sense, but because the plugin code is expected to dissapear eventually, and even if that is not the case, I wouldn't add that changes to this PR, this PR is ready to merge (from my point of view).

…tecture

mgdelacroix

Terrific job! 🚀

mgdelacroix · 2022-07-12T07:46:21Z

mattermost-plugin/product/api_adapter.go

+}
+
+// Ensure the adapter implements ServicesAPI.
+var _ model.ServicesAPI = &serviceAPIAdapter{}


mgdelacroix · 2022-07-14T11:19:33Z

mattermost-plugin/server/boards/configuration_test.go

-		p.SetAPI(api)
-		p.server = th.Server
-		p.wsPluginAdapter = &FakePluginAdapter{}
+		ctrl := gomock.NewController(t)


We're missing a ctrl.Finish() here or the expectations will not get checked

In the comments for NewController it says that Finish does not need to be called.

// NewController returns a new Controller. It is the preferred way to create a // Controller. // // New in go1.14+, if you are passing a *testing.T into this function you no // longer need to call ctrl.Finish() in your test methods.

https://github.com/golang/mock/blob/73266f9366fcf2ccef0b880618e5a9266e4136f4/gomock/controller.go#L87

mgdelacroix · 2022-07-14T11:21:27Z

mattermost-plugin/server/boards/data_retention_test.go

+	tearDown := func() {
+		os.Setenv("FOCALBOARD_UNIT_TESTING", origUnitTesting)
+	}
+
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()


Unrelated to this PR, but the ctrl.Finish() should get executed as part of the teardown

I moved it to the teardown, but I think it may be redundant.

mgdelacroix · 2022-07-14T11:22:55Z

mattermost-plugin/server/boards/mutex_adapter.go

+	"errors"
+	"net/http"
+
+	mm_model "github.com/mattermost/mattermost-server/v6/model"


Maybe not for this PR, but we're referring to the Mattermost Model as both mm_model and mmModel. We should pick one and standardize

Code debt ticket created: https://community.mattermost.com/boards/workspace/qgsck6cts3fwpqwyjiupjm5cde/b4ctmm917q3rzdfi451qtuxjzcw/v4mi5d7u6afbhfrks5783gbmj9o/csz137h5mjtgj3b68xtu5cs5zqr

mgdelacroix · 2022-07-14T11:31:57Z

server/app/cloud_test.go

@@ -649,7 +649,8 @@ func TestNotifyPortalAdminsUpgradeRequest(t *testing.T) {
 	defer tearDown()

 	t.Run("should send message", func(t *testing.T) {
-		pluginAPI := &plugintest.API{}
+		ctrl := gomock.NewController(t)


Suggested change

ctrl := gomock.NewController(t)

ctrl := gomock.NewController(t)

defer ctrl.Finish()

mgdelacroix · 2022-07-14T11:32:14Z

server/app/cloud_test.go

@@ -691,17 +692,18 @@ func TestNotifyPortalAdminsUpgradeRequest(t *testing.T) {
 	})

 	t.Run("no sys admins found", func(t *testing.T) {
-		pluginAPI := &plugintest.API{}
+		ctrl := gomock.NewController(t)


Suggested change

ctrl := gomock.NewController(t)

ctrl := gomock.NewController(t)

defer ctrl.Finish()

mgdelacroix · 2022-07-14T11:32:28Z

server/app/cloud_test.go

@@ -714,7 +716,8 @@ func TestNotifyPortalAdminsUpgradeRequest(t *testing.T) {
 	})

 	t.Run("iterate multiple pages", func(t *testing.T) {
-		pluginAPI := &plugintest.API{}
+		ctrl := gomock.NewController(t)


Suggested change

ctrl := gomock.NewController(t)

ctrl := gomock.NewController(t)

defer ctrl.Finish()

mgdelacroix · 2022-07-14T11:46:40Z

server/services/store/mattermostauthlayer/mattermostauthlayer.go

+type NotSupportedError struct {
+	msg string
+}
+
+func (pe NotSupportedError) Error() string {
+	return pe.msg
+}


I don't think this is being used right now, and duplicates the one on store.go

…tecture

wiggin77 · 2022-07-14T21:18:38Z

Thanks for reviewing @mgdelacroix. I've addressed the issued your raised, except the mock controller finish calls. I don't think they are needed anymore (since Go v1.14).

mgdelacroix

Thanks for applying the changes!! LGTM and merging right away!

As per the Finish() comments, you're totally right, they're not needed on our current version of Go. We still need to fix the test helpers in which we're calling defer ctrl.Finish() on the setup, but nothing concerning this PR. Will update the code debt card to add your findings

This reverts commit 605c007.

mgdelacroix · 2022-07-15T11:48:51Z

I merged the PR before the branch cut, so it's now reverted on main. Will merge it again after the release cut

wiggin77 added 19 commits May 24, 2022 13:21

skeleton lifecycle

eb5b74c

fix merge conflicts

d5f9aa4

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

339b905

…tecture

bare minimum to satisfy mm-server import

b9705d6

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

8a77f60

…tecture

added boards_imports.go

6f19f78

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

b212923

…tecture

move boards_imports.go to correct package

1cf0914

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

c8fcb52

…tecture

bump mmserver version; remove replace in go.mod; use module workspace…

d536897

…s; remove logger service

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

1b3a7c0

…tecture

rename product.go --> boards.go

327f248

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

d0eed72

…tecture

add FileInfoStore and Cloud services for product; create minimal plug…

40a5d98

…inAPI interfaces for all packages

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

95d2844

…tecture

rename Boards -> BoardsProduct

f1525a5

compile success

7da66be

fix merge conflicts

045bc21

remove hooks service; guard for nil BoardsApp

3fb9c57

wiggin77 requested a review from a team as a code owner July 1, 2022 19:38

wiggin77 requested review from sbishel and harshilsharma63 and removed request for a team July 1, 2022 19:38

wiggin77 marked this pull request as draft July 1, 2022 19:38

wiggin77 removed request for sbishel and harshilsharma63 July 1, 2022 19:38

wiggin77 added 4 commits July 1, 2022 16:24

update to latest mmserver ver

ed89e72

upgrade mmserver to master tip

6fb73af

upgrade mmserver to master tip

b9c89d1

bump plugin-api to master tip

2d84058

wiggin77 added 2 commits July 6, 2022 09:55

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

54b6f59

…tecture

partially disable cypress test

d1047e3

wiggin77 marked this pull request as ready for review July 6, 2022 16:52

wiggin77 requested review from mgdelacroix and jespino July 6, 2022 16:52

wiggin77 added the 2: Dev Review Requires review by a core committer label Jul 6, 2022

wiggin77 added this to the v7.3 milestone Jul 6, 2022

wiggin77 requested a review from sbishel July 6, 2022 17:03

jespino approved these changes Jul 7, 2022

View reviewed changes

wiggin77 added 4 commits July 7, 2022 17:52

fix merge conflicts

fc14d56

fix case Id --> ID

72e481c

bump mmserver version

852cb21

fix merge conflicts

cad06c4

mgdelacroix added the Do Not Merge Should not be merged until this label is removed label Jul 12, 2022

wiggin77 added 3 commits July 12, 2022 12:56

include in go.work for dev

c9bd0bb

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

fe96048

…tecture

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

f3ebd34

…tecture

mgdelacroix reviewed Jul 14, 2022

View reviewed changes

wiggin77 added 2 commits July 14, 2022 16:58

Merge remote-tracking branch 'upstream/main' into multi-product-archi…

56e1135

…tecture

addressed review comments.

ed7a742

mgdelacroix approved these changes Jul 15, 2022

View reviewed changes

mgdelacroix added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer Do Not Merge Should not be merged until this label is removed labels Jul 15, 2022

mgdelacroix merged commit 605c007 into mattermost:main Jul 15, 2022

mgdelacroix added a commit that referenced this pull request Jul 15, 2022

Revert "Multi product architecture (#3309)"

9456698

This reverts commit 605c007.

mgdelacroix added a commit that referenced this pull request Jul 15, 2022

Revert "Multi product architecture (#3309)" (#3373)

d093eef

This reverts commit 605c007.

mgdelacroix mentioned this pull request Jul 18, 2022

Multi product architecture #3381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi product architecture #3309

Multi product architecture #3309

wiggin77 commented Jul 1, 2022 •

edited

jespino left a comment

mgdelacroix left a comment

mgdelacroix Jul 12, 2022

mgdelacroix Jul 14, 2022

wiggin77 Jul 14, 2022

mgdelacroix Jul 14, 2022

wiggin77 Jul 14, 2022

mgdelacroix Jul 14, 2022

wiggin77 Jul 14, 2022

mgdelacroix Jul 14, 2022

mgdelacroix Jul 14, 2022

mgdelacroix Jul 14, 2022

mgdelacroix Jul 14, 2022

wiggin77 Jul 14, 2022

wiggin77 commented Jul 14, 2022

mgdelacroix left a comment

mgdelacroix commented Jul 15, 2022

	ctrl := gomock.NewController(t)
	ctrl := gomock.NewController(t)
	defer ctrl.Finish()

Multi product architecture #3309

Multi product architecture #3309

Conversation

wiggin77 commented Jul 1, 2022 • edited

Summary

Ticket Link

jespino left a comment

Choose a reason for hiding this comment

mgdelacroix 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

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

wiggin77 commented Jul 14, 2022

mgdelacroix left a comment

Choose a reason for hiding this comment

mgdelacroix commented Jul 15, 2022

wiggin77 commented Jul 1, 2022 •

edited