-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
microsoft teams app identifier #26996
microsoft teams app identifier #26996
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. |
sdk/communication/communication-common/review/communication-common.api.md
Outdated
Show resolved
Hide resolved
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.
Lgtm! (with nits in the Changelog)
*/ | ||
microsoftBot?: SerializedMicrosoftBotIdentifier; | ||
microsoftTeamsApp?: SerializedMicrosoftTeamsAppIdentifier; |
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.
@AikoBB Are we adding this new identifier type to the common API spec and let all other ACS services know of the new type?
https://github.com/Azure/azure-rest-api-specs/blob/eb29c07a0f08110c932b384d5dc41241dc0b03ab/specification/communication/data-plane/Common/stable/2022-07-13/common.json#L159
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.
yes, we have work items User Story 3372933: [Common] Post-release work item, User Story 3253259: [Common SDK] Update Common REST API specs for GA under our feature where we will apply the same changes to the rest of the platforms. So it will be done in the scope of these user stories after applying the same changes and releasing other 5 platforms
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.
Hi @DominikMe @AikoBB Chat Signaling Client also has Communication Identifier model in API which is not dependent on common package. Could you please also create the work item for our team to update the model in signaling client?
@DominikMe Do you suggest to make @azure/communictaion-signaling consume common package directly to avoid the model inconsistency?
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.
Hi @LuChen-Microsoft! I mentioned in the user story description to ping your team.
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.
@LuChen-Microsoft Let's chat more about adding the dependency to common. I'm trying to remember why we had avoided to add it in the past
Packages impacted by this PR
@azure/communication-common
Issues associated with this PR
Describe the problem that is addressed by this PR
Change MicrosoftBotIdentifier to MicrosoftTeamsAppIdentifier for 3.0.0 GA
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
No need to add more UTs, current UTs are ok. I've modified UTs in this PR
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists