Add stable version of common model swagger for Azure Communication Services #12799
Conversation
DominikMe
requested review from
juancamilor,
RezaJooyandeh,
alexandra142,
whisper6284,
ajaych-ms and
amrelroumy-msft
|
Hi, @DominikMe Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vsswagger@microsoft.com |
Swagger Validation Report
|
| Rule | Message |
|---|---|
|
The new version is missing a property found in the old version. Was 'innerError' renamed or removed? New: Microsoft.CommunicationServicesCommon/stable/2021-03-07/common.json#L30:7 Old: Microsoft.CommunicationServicesIdentity/stable/2021-03-07/CommunicationIdentity.json#L223:7 |
️️✔️LintDiff succeeded [Detail] [Expand]
Validation passes for LintDiff.
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️⚠️[Staging] Cross Version BreakingChange (Base on preview version): 8 Warnings warning [Detail]
- Compared Swaggers (Based on Oad v0.8.6)
- original: preview/2020-11-19-preview1/common.json <---> new: stable/2021-03-07/common.json
️️✔️[Staging] Cross Version BreakingChange (Base on stable version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️🔄[Staging] SDK Track2 Validation inProgress [Detail]
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
|
Hi @DominikMe, Your PR has some issues. Please fix the CI sequentially by following the order of
|
openapi-pipeline-app
bot
added
CI-FixRequiredOnFailure
and removed
CI-FixRequiredOnFailure
labels
|
@anuchandy The duplicate model collision is for the common error shape. The CommunicationIdentity.json will eventually $ref the type in common.json but currently has the type duplicated. What's the best course of action? |
...communication/data-plane/Microsoft.CommunicationServicesCommon/stable/2021-03-07/common.json
Show resolved
Hide resolved
| "modelAsString": true | ||
| } | ||
| }, | ||
| "CommunicationIdentifierModel": { |
There was a problem hiding this comment.
Would CallingApplication be brought back later?
There was a problem hiding this comment.
Yes, once we support it, we should add it. We removed it for now, as it does not seem to be used in any GA service and internally we have yet to finalize which format would be mapped to applications to avoid breaking changes in the SDK code. Once they are all decided, we can add it back as a non-breaking change
| "properties": { | ||
| "rawId": { | ||
| "type": "string", | ||
| "description": "Raw Id of the identifier. Optional in requests, required in responses." |
There was a problem hiding this comment.
assuming rawId would be required in events as well?
There was a problem hiding this comment.
Yes, any outbound from Communication Services would include the rawId in the body, but developers do not need to send them in their requests
| "id" | ||
| ], | ||
| "properties": { | ||
| "id": { |
There was a problem hiding this comment.
Will this "id" be full MRI or will it be the inside "8:acs:<stable_resource_id>_"?
| } | ||
| } | ||
| }, | ||
| "CommunicationUserIdentifierModel": { |
There was a problem hiding this comment.
The problem with eliminating explicit "kind" enum and having "optional" full objects, would result customers processing the events as :
if (eventData.CommunicationUser != null)
{
}
else if (eventData.PhoneNumber != null)
{
}
else if (eventData.MicrosoftTeamsUser != null)
{
}
etc.
Not a clean switch case. Also it will be difficult distinguish between an error case(all user representation objects are null) and new user object(which can be detected by default label in a switch case).
So would it be beneficial to include "kind" along with the specific objects?
There was a problem hiding this comment.
having kind helps with deserialziation, but for preparing requests, it is redundant information. Also we were thinking other violation could happen so we need to check the properties anyways. Eg, someone could set multiple properties. We can offer serialization/deserialization in our code to help the developers. Eg: Azure/azure-sdk-for-net#18389
There was a problem hiding this comment.
(all user representation objects are null)
👆🏻 is a valid case, all being null is equivalent to kind: "unknown".
...communication/data-plane/Microsoft.CommunicationServicesCommon/stable/2021-03-07/common.json
Show resolved
Hide resolved
openapi-pipeline-app
bot
added
BreakingChangeReviewRequired
and removed
CI-FixRequiredOnFailure
labels
|
Hi @DominikMe, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review. |
|
The breaking change isn't breaking. @anuchandy Can you help us getting this merged? |
|
Synced with Dominik, like he mentioned the property name "innererror" is used as per the guideline ref. |
|
@ johanste could you provide the approval? Justification:
|
|
@JeffreyRichter, per the rationale above (effectively false positive since nothing has been shipped/published yet) I've marked this as "breaking change approved". Please remove it if you disagree.... |
johanste
added
the
Approved-BreakingChange
|
I agree. |
…rvices (Azure#12799) * Add stable version of common model swagger for Azure Communication Services * address comments * fix title in readme * reference common error models in identity swagger
This common.json contains common model definitions for Errors and Identifiers (i.e. participants in communications) that will then be referenced by other Azure Communication Services, such as the Chat service.
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Please ensure to add changelog with this PR by answering the following questions.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.