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-55] Update plugin with respect to phase 1 upgrades #333

Merged
merged 10 commits into from
Mar 14, 2024
Merged

Conversation

ayusht2810
Copy link
Contributor

@ayusht2810 ayusht2810 commented Dec 21, 2023

Summary

Updated plugin with respect to phase 1 upgrades

Ticket Link

@ayusht2810 ayusht2810 self-assigned this Dec 21, 2023
@ayusht2810 ayusht2810 added the 2: Dev Review Requires review by a core committer label Dec 21, 2023
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 18.47%. Comparing base (5d27c3d) to head (166bc8d).

Files Patch % Lines
server/http.go 0.00% 2 Missing ⚠️
server/configuration.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   18.41%   18.47%   +0.06%     
==========================================
  Files           9        9              
  Lines        1515     1510       -5     
==========================================
  Hits          279      279              
+ Misses       1185     1180       -5     
  Partials       51       51              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


Use `make dist` to build distributions of the plugin that you can upload to a Mattermost server for testing.

Use `make check-style` to check the style for the whole plugin.
This plugin contains both a server and web app portion. Read our documentation about the [Developer Workflow](https://developers.mattermost.com/integrate/plugins/developer-workflow/) and [Developer Setup](https://developers.mattermost.com/integrate/plugins/developer-setup/) for more information about developing and extending plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome 👍

@@ -82,7 +57,7 @@ func findManifest() (*model.Manifest, error) {
// we don't want to accidentally clobber anything we won't preserve.
var manifest model.Manifest
decoder := json.NewDecoder(manifestFile)
// decoder.DisallowUnknownFields() // Commented out until circleci image is updated: https://github.com/mattermost/mattermost-plugin-zoom/pull/269#discussion_r927075701
decoder.DisallowUnknownFields()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +145 to +152
api.On("KVSetWithOptions", "mutex_mmi_bot_ensure", mock.AnythingOfType("[]uint8"), model.PluginKVSetOptions{Atomic: true, OldValue: []uint8(nil), ExpireInSeconds: 15}).Return(true, nil)
api.On("KVSetWithOptions", "mutex_mmi_bot_ensure", []byte(nil), model.PluginKVSetOptions{ExpireInSeconds: 0}).Return(true, nil)

api.On("EnsureBotUser", &model.Bot{
Username: botUserName,
DisplayName: botDisplayName,
Description: botDescription,
}).Return(botUserID, nil)
Copy link
Contributor

@mickmister mickmister Dec 22, 2023

Choose a reason for hiding this comment

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

This changed because we made the code more hardened for race conditions. I wonder why there was no call for EnsureBotUser before the changes in this PR though. I wonder if there is any risk on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister What risk might you be talking about on this?

@mickmister
Copy link
Contributor

Hi @ayusht2810, can you please resolve the conflicts here? Thank you

@ayusht2810
Copy link
Contributor Author

@mickmister resolved the conflicts. Please re-review.

@mickmister mickmister added 3: QA Review Requires review by a QA tester 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 14, 2024
@mickmister mickmister merged commit 7fa5345 into master Mar 14, 2024
11 checks passed
@mickmister mickmister deleted the MM-55 branch March 14, 2024 06:39
@ayusht2810 ayusht2810 mentioned this pull request Apr 16, 2024
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.

2 participants