-
Notifications
You must be signed in to change notification settings - Fork 11
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
Adding the remoteID migration, and making the synthetic user identification more robust #539
Merged
Merged
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
4667028
Adding the remoteID migration, and making the synthetic user identifi…
jespino 16405eb
Adding missing file
jespino 95fa22f
Changing the migration to squirrel
jespino 730d229
Adding a final _ character to the msteams filter in the migration
jespino 4a24032
Fixing CI
jespino 3993583
Always getting a valid remote ID
jespino fb1c7ac
Fixing CI
jespino 0b0d667
Merge remote-tracking branch 'origin/main' into migrate-remote-id
jespino 9f43c41
Fixing CI
jespino 85ab922
Fixing CI
jespino 3b34f56
Addressing PR review comments
jespino 4bc9f35
Fixing CI
jespino 737f273
Addressing PR review comments
jespino File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you confirm if this check is needed? I would like to always register the SharedChannels for the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wiggin77 ☝️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want an easy way to to turn this off in case of a problem in production. However we don't want to use a fake remoteID either since that may be hard to clean up later. I would suggest calling
RegisterPluginForSharedChannels
always, but then callingUnregisterPluginForSharedChannels
ifp.getConfiguration().DisableSyncMsg == true
. That way you will have the real remoteID.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is the remoteID changing over disabling/enabling. Is the remote id going to be stable or it is going to change every time that I unregister the plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I applied the changes that you proposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine for our current purposes, but thinking (very) long term, I could also see us supporting multiple Microsoft tenants concurrently which might necessitate a distinct
remoteID
for each (if we wanted to, for example, distinguish each in the UI?)Stepping back, I'm also just wanting to confirm that /we/ (the plugin) don't really care about the value here at all, and that it's just an artifact of the way the shared channels subsystem works with users that happen to have a
RemoteID
set. In other words, we want all DMs and all GMs for all users, regardless of whether anyone has aRemoteID
set. Does that remain accurate @wiggin77?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you set the appropriate flag when registering then yes, the MS Teams plugin will get sync for all DM/GMs regardless of remoteID value. Other plugins using the API will likely not set that flag and the remoteID on posts/users/etc will matter.
Should we modify the register API to accept a remoteID, thus allowing a plugin to call register for each tenant? That would also leave it up to the plugin to ensure idempotency. cc @jespino @lieut-data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the one hand, I don't want to preemptively build for the future. But I do like the idea of giving the plugin control over the "primary key" backing the cursor based model. At the moment, it feels just a bit too decoupled. So I'm in favour of the idea. Is the
RemoteId
constrained by some length we should remain aware of? (i.e. is it long enough for a UUID?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same length as most IDs in MM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circling back on this from a discussion with @jespino: I'm agreeing with his proposal to just use the
RemoteID
as-is and deal with the multi-tenant situation in the future (if needed). This unblocks things and gets us closer to being able to rely on shared channels for message delivery. So essentially no changes required on this thread.