search

mattermost-community / mattermost-plugin-jitsi

Jitsi plugin for Mattermost :electric_plug:
Apache License 2.0
193 stars 89 forks source link

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

Closed ayusht2810 closed 6 months ago

ayusht2810 commented 7 months ago

Summary

Updated plugin with respect to phase 1 upgrades

Ticket Link

VincentSC commented 6 months ago

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://git@github.com/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
VincentSC commented 6 months ago

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

/go/pkg/mod/google.golang.org/grpc@v1.60.0/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 commented 6 months ago

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/v6@v6.7.2/shared/mlog/mlog.go:87:11: cannot use generic function logr.Int without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/v6@v6.7.2/shared/mlog/mlog.go:96:12: cannot use generic function logr.Uint without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/v6@v6.7.2/shared/mlog/mlog.go:105:14: cannot use generic function logr.String without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/v6@v6.7.2/shared/mlog/mlog.go:130:12: cannot use generic function logr.Bool without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/v6@v6.7.2/shared/mlog/mlog.go:143:13: cannot use generic function logr.Array without instantiation
/root/go/pkg/mod/github.com/mattermost/mattermost-server/v6@v6.7.2/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 commented 6 months ago

/update-branch

mickmister commented 6 months ago

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://git@github.com/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 commented 6 months ago

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 commented 6 months ago

@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://git@github.com/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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Yes, it works!

VincentSC commented 6 months ago

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

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