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

[MM-39399] Bump min_server_version due to imported server dependency breaking changes #236

Merged
merged 5 commits into from
Feb 9, 2022

Conversation

mickmister
Copy link
Contributor

@mickmister mickmister commented Oct 19, 2021

Summary

There is a breaking change with 5.37 server code, where the plugin will crash if:

  • The plugin is importing code 5.37 or later
  • The server is running 5.36 or before

This PR downgrades the dependencies of mattermost-plugin-api and mattermost-server. The only change to the plugin source code is a change in the constructor of the pluginapi.Client. We are no longer accessing a plugin.Driver struct to pass to this constructor.

The dependency was originally bumped for the license check. The license check code is very small and has no dependencies, so I copied some code over to this repo to avoid the greater dependency. The license check is still the correct logic.

Edit: I've changed the PR so it just updates the min_server_version

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-39399

@mickmister mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Oct 19, 2021
Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Should we bump the minimum server version to 5.37 to fix this issue? 5.36 is a quite old version that isn't supported any longer according to https://docs.mattermost.com/upgrade/extended-support-release.html.

@hanzei
Copy link
Collaborator

hanzei commented Oct 19, 2021

Do we need a patch release?

@codecov-commenter
Copy link

Codecov Report

Merging #236 (ee6b110) into master (9411708) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #236   +/-   ##
=======================================
  Coverage   23.80%   23.80%           
=======================================
  Files          62       62           
  Lines        2541     2541           
=======================================
  Hits          605      605           
  Misses       1857     1857           
  Partials       79       79           

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 9411708...ee6b110. Read the comment docs.

@mickmister mickmister changed the title [MM-39399] Downgrade server dependency to avoid panic [MM-39399] Bump min_server_version due to imported server dependency breaking changes Oct 19, 2021
@mickmister
Copy link
Contributor Author

Closing in favor of mattermost/mattermost-marketplace#233

@mickmister mickmister closed this Oct 19, 2021
@hanzei
Copy link
Collaborator

hanzei commented Oct 20, 2021

Don't we want to bump the min_server_version for the next release?

@mickmister
Copy link
Contributor Author

Repoening, as the fix still needs to be applied here, and not just the marketplace.

@mickmister mickmister reopened this Jan 26, 2022
@mickmister mickmister added this to the v1.2.0 milestone Jan 26, 2022
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Feb 8, 2022
@hanzei
Copy link
Collaborator

hanzei commented Feb 8, 2022

Let's simply merge this as the fix is straight forward.

@hanzei hanzei merged commit 7fa0a92 into master Feb 9, 2022
@hanzei hanzei deleted the downgrade-mm-server-dependency branch February 9, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants