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-44056] Make Calls plugin's state on Cloud controllable by feature flag #20161

Merged
merged 6 commits into from May 10, 2022

Conversation

streamer45
Copy link
Contributor

@streamer45 streamer45 commented May 9, 2022

Summary

Fortunately it looks like Apps solved our problem already so I am following a similar approach.

  • Calls will be pre-packaged and set to enabled by default which means it gets installed automatically on first server start.
  • Upon actual plugin's activation we check whether the CallsEnabled feature flag is set to true and only in that case we allow the plugin to be started.

None of this should impact on-prem as all the checks are guarded by the IsCloud util (leveraging MM_CLOUD_INSTALLATION_ID).

Since we are here, I also added the code to pre-package as it will make it easier to test this PR. Just bear in mind I may have to bump the plugin's version before merging to get the latest changes necessary for it to work properly on Cloud.

Ticket Link

https://mattermost.atlassian.net/browse/MM-44056

Release Note

Prepackage Calls v0.5.1

@streamer45 streamer45 added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels May 9, 2022
@streamer45 streamer45 self-assigned this May 9, 2022
@mm-cloud-bot mm-cloud-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 9, 2022
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

Nice! Just one suggestion below.

app/plugin.go Outdated Show resolved Hide resolved
app/plugin_install.go Outdated Show resolved Hide resolved
@@ -32,12 +32,16 @@ type FeatureFlags struct {
PluginPlaybooks string `plugin_id:"playbooks"`
PluginApps string `plugin_id:"com.mattermost.apps"`
PluginFocalboard string `plugin_id:"focalboard"`
PluginCalls string `plugin_id:"com.mattermost.calls"`
Copy link
Member

Choose a reason for hiding this comment

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

I /think/ most of these are now unused, but no objection to extending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, wasn't sure either but given there was a check that made use of those in installFeatureFlagPlugins I opted to include it although it shouldn't have any impact on the PR's scope.

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Thanks Claudio, looks great!

streamer45 and others added 2 commits May 9, 2022 22:04
Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
@streamer45
Copy link
Contributor Author

/e2e-test

@mattermod
Copy link
Contributor

@streamer45 streamer45 added the Setup Cloud Test Server Setup an on-prem test server label May 10, 2022
@streamer45
Copy link
Contributor Author

@saturninoabril Could you suggest someone to have a quick look at this from a QA perspective? If possible, we are looking to merge this asap to get into tomorrow's Cloud release.

Main things to check would be:

  • Calls is installed but not enabled by default. No UI related to Calls should be visible to the user at this point.
  • Switching the CallsEnabled feature flag to true enables the plugin.
  • Switching the CallsEnabled feature flag to false disables the plugin.
  • Apps is enabled by default.
  • Switching the AppsEnabled feature flag to false disables the plugin.

/cc @amyblais

@streamer45 streamer45 added the 2: Infra Review Requires review by a SRE label May 10, 2022
@streamer45
Copy link
Contributor Author

@angeloskyratzakos This landing on Community will likely mean we need to have the flag on in order for Calls to remain enabled. So it's about setting MM_FEATUREFLAGS_CALLSENABLED=true . Let me know if you have questions/concerns.

@saturninoabril
Copy link
Member

Claudio, I'll take a look.

@saturninoabril saturninoabril self-requested a review May 10, 2022 10:11
@streamer45
Copy link
Contributor Author

Claudio, I'll take a look.

Amazing, thank you 🙌

@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label May 10, 2022
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Thanks @streamer45! Tested and passed.

@saturninoabril saturninoabril added QA/Review Done QA review is completed but other reviews are outstanding (exception to usual process) and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels May 10, 2022
@streamer45 streamer45 removed the 2: Infra Review Requires review by a SRE label May 10, 2022
@streamer45 streamer45 added 4: Reviews Complete All reviewers have approved the pull request Do Not Merge Should not be merged until this label is removed labels May 10, 2022
@streamer45 streamer45 removed the Do Not Merge Should not be merged until this label is removed label May 10, 2022
@streamer45
Copy link
Contributor Author

/update-branch

@streamer45 streamer45 merged commit aee68c7 into master May 10, 2022
@streamer45 streamer45 deleted the MM-44056 branch May 10, 2022 17:29
@mattermod mattermod removed the Setup Cloud Test Server Setup an on-prem test server label May 10, 2022
@mm-cloud-bot
Copy link

Test server destroyed

1 similar comment
@mm-cloud-bot
Copy link

Test server destroyed

@streamer45
Copy link
Contributor Author

/cherry-pick cloud

@mattermod
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit to mattermost-build/mattermost that referenced this pull request May 10, 2022
… flag (mattermost#20161)

* Make Calls plugin's state on Cloud controllable by feature flag

* Prepackage Calls v0.4.9 (mattermost#20148)

* Update app/plugin.go

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

* Update app/plugin_install.go

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

* Bump Calls version to latest

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
(cherry picked from commit aee68c7)
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels May 10, 2022
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels May 10, 2022
mattermod added a commit to mattermost-build/mattermost that referenced this pull request May 10, 2022
mattermod added a commit to mattermost-build/mattermost that referenced this pull request May 10, 2022
amyblais pushed a commit that referenced this pull request May 10, 2022
… flag (#20161) (#20174)

* Make Calls plugin's state on Cloud controllable by feature flag

* Prepackage Calls v0.4.9 (#20148)

* Update app/plugin.go

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

* Update app/plugin_install.go

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

* Bump Calls version to latest

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
(cherry picked from commit aee68c7)

Co-authored-by: Claudio Costa <cstcld91@gmail.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
oh6hay pushed a commit that referenced this pull request May 18, 2022
… flag (#20161)

* Make Calls plugin's state on Cloud controllable by feature flag

* Prepackage Calls v0.4.9 (#20148)

* Update app/plugin.go

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

* Update app/plugin_install.go

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

* Bump Calls version to latest

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
@amyblais amyblais added Docs/Needed Requires documentation and removed Docs/Not Needed Does not require documentation labels May 27, 2022
@amyblais amyblais added this to the v7.0 milestone May 27, 2022
@justinegeffen justinegeffen added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Jun 14, 2022
FreedomBen pushed a commit to FreedomBen/mattermost-server that referenced this pull request Jun 14, 2022
… flag (mattermost#20161)

* Make Calls plugin's state on Cloud controllable by feature flag

* Prepackage Calls v0.4.9 (mattermost#20148)

* Update app/plugin.go

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

* Update app/plugin_install.go

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

* Bump Calls version to latest

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
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 Changelog/Done Required changelog entry has been written CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Done Required documentation has been written QA/Review Done QA review is completed but other reviews are outstanding (exception to usual process) release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants