[MM-62] Update plugin with respect to phase 1 upgrades #237

ayusht2810 · 2023-12-28T14:21:40Z

Summary

Updated plugin with respect to phase 1 upgrades

Updated go version from 1.12 to 1.19
Fixed build issues being created from dockerfile
Updated build folder to sync with starter template
Updated package in go modules to use https://pkg.go.dev/github.com/mattermost/mattermost/server/public
Updated Makefile
Updated version in nvmrc file and update package-lock.json file.
Added base setup for test cases
Fix lint errors
Updated developer docs link in the readme file

Ticket Link

VincentSC · 2024-01-07T23:38:19Z

Feedback. Still did not get it working, but it's already a big improvement on what is there. See also #206

The first set of errors could be solved by updating the first line of Dockerfile to FROM golang:1.18. This is in line with what go.mod defines.

I got this error:

npm WARN tarball tarball data for redux-offline@git+ssh://[email protected]/enahum/redux-offline.git#885024de96b6ec73650c340c8928066585c413df (sha512-srmJ1vWm8ZQTYflZCf7oUs3WBX83GyCIzsFUpwxUg2wcDHngSHjjShRTCgmkciPkVmM4aJ33i9baYS9jRC+zLA==) seems to be corrupted.
npm ERR! code EINTEGRITY
npm ERR! sha512-srmJ1vWm8ZQTYflZCf7oUs3WBX83GyCIzsFUpwxUg2wcDHngSHjjShRTCgmkciPkVmM4aJ33i9baYS9jRC+zLA== integrity checksum failed when using sha512: wanted sha512-srmJ1vWm8ZQTYflZCf7oUs3WBX83GyCIzsFUpwxUg2wcDHngSHjjShRTCgmkciPkVmM4aJ33i9baYS9jRC+zLA== but got sha512-SThdHzd9uoZ4Uo6I1X4fc1Q1aU5erk6TRZdRVdRtkHrIaYzVhcNqZYGg/euh+LVfiAWQIo6WPeKuGgrZVghBSw==. (49960 bytes)

This could be solved by removing package-lock.json.

Running golangci-lint
golangci-lint run ./...
ERRO Running error: no such linter "revive"       
make: *** [Makefile:52: check-style] Error 3

Needs to be solved in Makefile, where a really old version is used. Put it to v1.55.2. And that triggered something, I think. :)

golangci-lint run ./...
server/api.go:89:23: p.API undefined (type *Plugin has no field or method API) (typecheck)
	bundlePath, err := p.API.GetBundlePath()
	                     ^
server/api.go:160:20: p.API undefined (type *Plugin has no field or method API) (typecheck)
	user, appErr := p.API.GetUser(userID)
	                  ^
server/api.go:190:17: p.API undefined (type *Plugin has no field or method API) (typecheck)
	if _, err := p.API.GetChannelMember(channelID, userID); err != nil {
	               ^
server/command_test.go:30:4: p.SetAPI undefined (type Plugin has no field or method SetAPI) (typecheck)
	p.SetAPI(&apiMock)
	  ^
server/command_test.go:32:39: p.API undefined (type Plugin has no field or method API) (typecheck)
	i18nBundle, err := i18n.InitBundle(p.API, filepath.Join("assets", "i18n"))
	                                     ^
server/command_test.go:119:6: p.SetAPI undefined (type Plugin has no field or method SetAPI) (typecheck)
			p.SetAPI(&apiMock)
			  ^
server/command_test.go:124:41: p.API undefined (type Plugin has no field or method API) (typecheck)
			i18nBundle, err := i18n.InitBundle(p.API, filepath.Join("assets", "i18n"))
			                                     ^
server/command_test.go:157:5: p.SetAPI undefined (type Plugin has no field or method SetAPI) (typecheck)
		p.SetAPI(&apiMock)
		  ^
server/command_test.go:164:40: p.API undefined (type Plugin has no field or method API) (typecheck)
		i18nBundle, err := i18n.InitBundle(p.API, filepath.Join("assets", "i18n"))
		                                     ^
make: *** [Makefile:52: check-style] Error 1

larkox

Codewise, LGTM.

VincentSC · 2024-01-08T12:39:57Z

Building needs Go 1.19 or higher, else it will stop on:

/go/pkg/mod/google.golang.org/[email protected]/server.go:2161:14: undefined: atomic.Int64

Also the lint-errors found with newer versions of golangci-lint need attention. Depending on the used version, different problems arise. This is caused by a change in go.mod:

google.golang.org/grpc v1.60.0 // indirect

Even with that disabled, it fails on the existing tests of ./webapp/src/components/post_type_jitsi
Here's one example:

 FAIL  src/components/conference/conference.test.tsx (5.437 s)
  Conference
    ✓ should render null if the post type is null (9 ms)
    ✕ should render and initialize the conference interface (17 ms)
    ✓ should execute the hangup command, wait and call the action to close the meeting, and reset the state on closed (488 ms)
    should show the correct buttons depending on the state
      ✕ should have down, open outside, maximize and close buttons (8 ms)
      ✕ should have up, open outside, maximize and close buttons (7 ms)
      ✕ should have open outside, minimize and close buttons (7 ms)
    should show the the loading spinner depending on the state
      ✕ should show loading (7 ms)
      ✕ should not show loading (6 ms)
    should maximize based on the state
      ✓ should toggle tile only if was open before minimize and now is closed (8 ms)
      ✓ should toggle filmstrip only if was open before minimize and now is closed (6 ms)
    should minimize based on the state
      ✓ should toggle tile only if is open before minimizing (3 ms)
      ✓ should toggle filmstrip only if is open before minimize (4 ms)

VincentSC · 2024-01-09T10:02:35Z

When updating the Mattermost API to V6 (or higher), I get:

# github.com/mattermost/mattermost-server/v6/shared/mlog
/root/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/shared/mlog/mlog.go:87:11: cannot use generic function logr.Int without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/shared/mlog/mlog.go:96:12: cannot use generic function logr.Uint without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/shared/mlog/mlog.go:105:14: cannot use generic function logr.String without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/shared/mlog/mlog.go:130:12: cannot use generic function logr.Bool without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/shared/mlog/mlog.go:143:13: cannot use generic function logr.Array without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/shared/mlog/mlog.go:146:11: cannot use generic function logr.Map without instantiation
FAIL	github.com/mattermost/mattermost-plugin-jitsi/server [build failed]
FAIL
make: *** [Makefile:193: test] Error 1

mickmister · 2024-01-09T16:21:56Z

/update-branch

mickmister · 2024-01-09T16:35:41Z

For some reason the CI checks are not running for this PR. This should catch the failing webapp test. @ayusht2810 Can you please run the ci commands locally while we figure out the issues with CI? Or have you run them and are not seeing the issues pointed out above?

@toninis Any thoughts on why CI may not be running here?

When updating the Mattermost API to V6 (or higher)

@VincentSC What were your steps for changing the dependency (update paths in source etc.)? This PR already updates the dependency to github.com/mattermost/mattermost/server/public v0.0.12 which is the recommended way to import the server dependencies now.

npm WARN tarball tarball data for redux-offline@git+ssh://[email protected]/enahum/redux-offline.git...

@VincentSC Is your git environment set up to allow for using ssh with GitHub?

The first set of errors could be solved by updating the first line of Dockerfile to FROM golang:1.18. This is in line with what go.mod defines.

I'm not sure where that Dockerfile came from. Looks like a contributor added that early on to help compile without installing Go. @ayusht2810 We should either remove the related docker files in the root dir of the repo or update the Go version in the Dockerfile. I propose removing them if we don't need them, as it's just another piece we need to remember update in this kind of PR that can be missed.

VincentSC · 2024-01-09T16:45:38Z

The Dockerfile is superhandy! It makes sure that an environment is as expected and triaging bugs is not caused by version-problems anywhere. So please don't remove!!

We have already fixed the Dockerfile by using Ubuntu 23.4 and various other things. Problem is that Go and NPM have their own dependency-systems and this is causing conflicts.

I prefer to replicate a working system, as I assume a non-documented command somewhere

VincentSC · 2024-01-11T10:52:35Z

@VincentSC What were your steps for changing the dependency (update paths in source etc.)? This PR already updates the dependency to github.com/mattermost/mattermost/server/public v0.0.12 which is the recommended way to import the server dependencies now.

Then the project uses both and needs a good cleanup.

npm WARN tarball tarball data for redux-offline@git+ssh://[email protected]/enahum/redux-offline.git...

@VincentSC Is your git environment set up to allow for using ssh with GitHub?

It is done via the docker-build. But this specific environment is indeed not set up for ssh, as I set up something temporary on a server here. Seemingly that is required and not documented? It's weird, as it does not need to be done this way, with https being available.

Also, the redux-offline project is not updated in years! Neither upstream, but not as bad as the one at mattermost.

I propose removing them if we don't need them, as it's just another piece we need to remember update in this kind of PR that can be missed.

My life should not about a single project that defines how my environment should look like. Docker solves that. We have some improvements for just the docker-build, by the way.

ayusht2810 · 2024-01-15T07:17:36Z

@VincentSC @mickmister I have fixed the build issues in the Docker file, and now we are able to create a build using the Docker file. Also, regarding the failing of webapp unit test cases, they are passing on my local.

VincentSC · 2024-01-15T07:51:52Z

I have fixed the build issues in the Docker file, and now we are able to create a build using the Docker file.
Great! Thanks! I'll test later today.

Does it still need ssh-access to Gitlab or will https suffice?

Also, regarding the failing of webapp unit test cases, they are passing on my local.
Do you mean on your machine, but within Docker?

ayusht2810 · 2024-01-15T08:21:28Z

Does it still need ssh-access to Gitlab or will https suffice?

@VincentSC I haven't tested that part.

Do you mean on your machine, but within Docker?

They were passing on my machine as well as within the Docker.

VincentSC · 2024-01-15T12:23:47Z

Yes, it works!

mickmister · 2024-01-16T22:55:12Z

docker-make

 sudo docker build -t mattermost-plugin-jitsi-builder .
-sudo docker run --rm -it -v $(pwd):/src -w /src mattermost-plugin-jitsi-builder make "$@"
+sudo docker run --rm -it -v $(pwd):/src -w /src mattermost-plugin-jitsi-builder make "$@"


It may make sense to have this in build/custom.mk to make it into a make command, like make build-docker. 0/5

VincentSC · 2024-01-17T06:19:18Z

Merged! I'm happy about this progress, as it enables a lot!

One small problem I just discovered: version is still 2.0.0.

[MM-62] Update plugin with respect to phase 1 upgrades

9224693

ayusht2810 requested a review from larkox as a code owner December 28, 2023 14:21

ayusht2810 requested a review from raghavaggarwal2308 December 28, 2023 14:22

ayusht2810 self-assigned this Dec 28, 2023

ayusht2810 added the 2: Dev Review Requires review by a core committer label Dec 28, 2023

[MM-62] Remove extra comment

6af4846

larkox approved these changes Jan 8, 2024

View reviewed changes

mickmister mentioned this pull request Jan 9, 2024

Update community supported README: legal disclaimer #236

Merged

[MM-62] Update docker file and lint issues

1a5b46a

VincentSC mentioned this pull request Jan 15, 2024

[GH-183] Add jaas support #219

Open

mickmister self-requested a review January 16, 2024 22:52

mickmister approved these changes Jan 16, 2024

View reviewed changes

mickmister merged commit c254d0c into master Jan 16, 2024

mickmister deleted the MM-62 branch January 16, 2024 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-62] Update plugin with respect to phase 1 upgrades #237

[MM-62] Update plugin with respect to phase 1 upgrades #237

ayusht2810 commented Dec 28, 2023 •

edited

Loading

VincentSC commented Jan 7, 2024

larkox left a comment •

edited

Loading

VincentSC commented Jan 8, 2024 •

edited by mickmister

Loading

VincentSC commented Jan 9, 2024 •

edited

Loading

mickmister commented Jan 9, 2024

mickmister commented Jan 9, 2024

VincentSC commented Jan 9, 2024 •

edited

Loading

VincentSC commented Jan 11, 2024 •

edited

Loading

ayusht2810 commented Jan 15, 2024

VincentSC commented Jan 15, 2024

ayusht2810 commented Jan 15, 2024 •

edited

Loading

VincentSC commented Jan 15, 2024

mickmister Jan 16, 2024

VincentSC commented Jan 17, 2024

[MM-62] Update plugin with respect to phase 1 upgrades #237

[MM-62] Update plugin with respect to phase 1 upgrades #237

Conversation

ayusht2810 commented Dec 28, 2023 • edited Loading

Summary

Ticket Link

VincentSC commented Jan 7, 2024

larkox left a comment • edited Loading

Choose a reason for hiding this comment

VincentSC commented Jan 8, 2024 • edited by mickmister Loading

VincentSC commented Jan 9, 2024 • edited Loading

mickmister commented Jan 9, 2024

mickmister commented Jan 9, 2024

VincentSC commented Jan 9, 2024 • edited Loading

VincentSC commented Jan 11, 2024 • edited Loading

ayusht2810 commented Jan 15, 2024

VincentSC commented Jan 15, 2024

ayusht2810 commented Jan 15, 2024 • edited Loading

VincentSC commented Jan 15, 2024

mickmister Jan 16, 2024

Choose a reason for hiding this comment

VincentSC commented Jan 17, 2024

ayusht2810 commented Dec 28, 2023 •

edited

Loading

larkox left a comment •

edited

Loading

VincentSC commented Jan 8, 2024 •

edited by mickmister

Loading

VincentSC commented Jan 9, 2024 •

edited

Loading

VincentSC commented Jan 9, 2024 •

edited

Loading

VincentSC commented Jan 11, 2024 •

edited

Loading

ayusht2810 commented Jan 15, 2024 •

edited

Loading