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.

Sign up for GitHub

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

Jump to bottom

MM-58438: simplify settings #688

Merged
merged 12 commits into from
Jun 14, 2024
Merged

MM-58438: simplify settings #688

merged 12 commits into from
Jun 14, 2024

Conversation

lieut-data
Copy link
Member

@lieut-data lieut-data commented Jun 4, 2024 •
edited
Loading

Summary

We're simplifying the plugin configuration to focus users towards the GA functionality for v10:

  • Remove the untested enabledTeams setting altogether
  • Remove syncReactions and always sync reactions (as appropriate)
  • Remove syncFileAttachments and always sync file attachments (as appropriate)
  • Consolidate syncDirectMessages and syncGroupMessages under a new experimentalSyncChat
  • Rename selectiveSync as experimentalSelectiveSync
    • Rename certificatepublic as experimentalcertificatepublic and certificatekey as experimentalcertificatekey.
  • Rename disableSyncMsg as experimentalUseSharedChannels so that it defaults off.
  • Rename disableCheckCredentials as internalDisableCheckCredentials
  • Remove all internal and experimental options from plugin.json to hide them from the system console.

Ticket Link

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

@lieut-data lieut-data changed the title Mm 58438 simplify settings MM-58438: simplify settings Jun 4, 2024
Base automatically changed from mm-58438-simplify-restrictive-sync to main June 5, 2024 17:15
@lieut-data lieut-data marked this pull request as ready for review June 6, 2024 15:55
@lieut-data lieut-data added the 2: Dev Review Requires review by a core committer label Jun 6, 2024
JulienTant
JulienTant approved these changes Jun 12, 2024
View reviewed changes
Copy link
Member

@JulienTant JulienTant left a comment

Choose a reason for hiding this comment

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

LGTM, I'm hesitant to say it's a good idea to re-merge DM and GM tho, I think our DM experience at this point is better than experimental but our GMs are not really at the same level.

server/handlers.go
@@ -332,7 +332,7 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs
return metrics.DiscardedReasonOther
}

post, skippedFileAttachments, errorFound := ah.msgToPost(channelID, senderID, msg, chat, ah.plugin.GetSyncFileAttachments(), []string{})
post, skippedFileAttachments, errorFound := ah.msgToPost(channelID, senderID, msg, chat, true, []string{})
Copy link
Member

@JulienTant JulienTant Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have cases where the attachements parameters is false? and if not, should we remove this parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Relative to this PR, yes: notifications currently don't process attachments. I have a separate (pending) PR to enable that, after which this parameter will go away.

@lieut-data
Copy link
Member Author

LGTM, I'm hesitant to say it's a good idea to re-merge DM and GM tho, I think our DM experience at this point is better than experimental but our GMs are not really at the same level.

I agree that GMs introduce the most experimental level of functionality, but the overhead to enable one without the other is non-trivial. Syncing with @esethna, we're aiming to deprioritize chat sync altogether, with no plan to "sell" only DMs to any customer.

@JulienTant
Copy link
Member

I agree that GMs introduce the most experimental level of functionality, but the overhead to enable one without the other is non-trivial. Syncing with @esethna, we're aiming to deprioritize chat sync altogether, with no plan to "sell" only DMs to any customer.

Sounds good then! Thanks!

sbishel
sbishel approved these changes Jun 12, 2024
View reviewed changes
Copy link
Member

@sbishel sbishel left a comment

Choose a reason for hiding this comment

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

Just a question as to naming.

server/configuration.go
@@ -46,8 +42,8 @@ type configuration struct {
SyntheticUserAuthService string `json:"syntheticUserAuthService"`
SyntheticUserAuthData string `json:"syntheticUserAuthData"`
AutomaticallyPromoteSyntheticUsers bool `json:"automaticallyPromoteSyntheticUsers"`
DisableSyncMsg bool `json:"disableSyncMsg"`
DisableCheckCredentials bool `json:"disableCheckCredentials"`
UseSharedChannels bool `json:"experimentalUseSharedChannels"`
Copy link
Member

Choose a reason for hiding this comment

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

Curious as to the word choice "Use". UseSharedChannels makes me think this is related to the SharedChannels architecture. (EnableSharedChannels). ShareChannels or AllowSharedChannels seems more appropriate. I'm 0/5 and may be misunderstanding.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sbishel, this is indeed referring to the shared channels architecture feature that won't be productized for v10. Agree the wording is confusing -- is there another term that might help clarify? Maybe even explicitly UseSharedChannelsArchitecture or EnableSharedChannelsArchitecture?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sbishel, just circling back to our conversation above -- any thoughts after the clarification?

Copy link
Member

Choose a reason for hiding this comment

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

If it is referring to the architecture, I think the current naming is fine, no need to add Architecture. Its use in plugin.go made me think it was more of an on/off flag rather than which architecture to use. My misunderstanding of its full purpose...only looking at the PR.

@lieut-data lieut-data merged commit 8456562 into main Jun 14, 2024
9 checks passed
@lieut-data lieut-data deleted the mm-58438-simplify-settings branch June 14, 2024 15:55
@lieut-data lieut-data added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbishel sbishel sbishel approved these changes

@JulienTant JulienTant JulienTant approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lieut-data @JulienTant @sbishel