-
Notifications
You must be signed in to change notification settings - Fork 5.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
Update ACS Email swagger based on prior feedback from review board in preparation for GA #21841
Conversation
Hi, @apattath 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. [email protected] |
Swagger Validation Report
|
Rule | Message |
---|---|
Runtime Exception |
"new":"https://github.com/Azure/azure-rest-api-specs/blob/2beb92d3d6c70d683183192bab3beac8bef09322/specification/communication/data-plane/Email/preview/2023-01-15-preview/CommunicationServicesEmail.json", "old":"https://github.com/Azure/azure-rest-api-specs/blob/main/specification/communication/data-plane/Email/preview/2021-10-01-preview/CommunicationServicesEmail.json", "details":"Command failed: node "/mnt/vss/_work/_tasks/AzureApiValidation_5654d05d-82c1-48da-ad8f-161b817f6d41/0.0.47/common/temp/node_modules/.pnpm/@Azure[email protected]/node_modules/autorest/dist/app.js" --v2 --input-file=specification/communication/data-plane/Email/preview/2023-01-15-preview/CommunicationServicesEmail.json --output-artifact=swagger-document.json --output-artifact=swagger-document.map --output-file=new --output-folder=/tmp\nERROR: Schema violation: Data does not match any schemas from 'oneOf'\n - file:///mnt/vss/_work/1/azure-rest-api-specs/specification/communication/data-plane/Email/preview/2023-01-15-preview/CommunicationServicesEmail.json:152:10 ( |
️️✔️
CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️
LintDiff: 6 Warnings warning [Detail]
compared tags (via openapi-validator v2.0.0) | new version | base version |
---|---|---|
package-2023-01-15-preview | package-2023-01-15-preview(2beb92d) | default(main) |
[must fix]The following errors/warnings are introduced by current PR:
Rule | Message | Related RPC [For API reviewers] |
---|---|---|
Path parameter should specify a maximum length (maxLength) and characters allowed (pattern). Location: Email/preview/2023-01-15-preview/CommunicationServicesEmail.json#L20 |
||
The error property in the error response schema should be required.Location: Email/preview/2023-01-15-preview/CommunicationServicesEmail.json#L40 |
||
Error schema should define code and message properties as required.Location: Email/preview/2023-01-15-preview/CommunicationServicesEmail.json#L40 |
||
The error property in the error response schema should be required.Location: Email/preview/2023-01-15-preview/CommunicationServicesEmail.json#L125 |
||
Error schema should define code and message properties as required.Location: Email/preview/2023-01-15-preview/CommunicationServicesEmail.json#L125 |
||
Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum. Location: Email/preview/2023-01-15-preview/CommunicationServicesEmail.json#L237 |
️️✔️
Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️
ApiReadinessCheck succeeded [Detail] [Expand]
️⚠️
~[Staging] ServiceAPIReadinessTest: 0 Warnings warning [Detail]
API Test is not triggered due to precheck failure. Check pipeline log for details.
️️✔️
SwaggerAPIView succeeded [Detail] [Expand]
️️✔️
CadlAPIView succeeded [Detail] [Expand]
️️✔️
ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️
SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️
PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️
PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️
SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️
Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️
CadlValidation succeeded [Detail] [Expand]
Validation passes for CadlValidation.
️️✔️
PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
Swagger pipeline restarted successfully, please wait for status update in this comment. |
Generated ApiView
|
Hi @apattath, Your PR has some issues. Please fix the CI sequentially by following the order of
|
...on/communication/data-plane/Email/preview/2021-10-01-preview/CommunicationServicesEmail.json
Outdated
Show resolved
Hide resolved
specification/communication/data-plane/Email/preview/2021-10-01-preview/examples/SendEmail.json
Outdated
Show resolved
Hide resolved
...on/communication/data-plane/Email/preview/2023-15-01-preview/CommunicationServicesEmail.json
Outdated
Show resolved
Hide resolved
...on/communication/data-plane/Email/preview/2023-15-01-preview/CommunicationServicesEmail.json
Outdated
Show resolved
Hide resolved
Hi, @apattath, For review efficiency consideration, when creating a new api version, it is required to place API specs of the base version in the first commit, and push new version updates into successive commits. You can use OpenAPIHub to initialize the PR for adding a new version. For more details refer to the wiki. Or you could onboard API spec pipeline |
Hi @apattath, 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 Changes PR check is failing because of a bug in openapi-diff, so I will add the Approved-BreakingChange label. |
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.
Looks good! 👍
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@mikekistler - could you help merge this PR please? |
@DominikMe @weidongxu-microsoft are you satisfied with the resolution to your comments? Is this good to merge? If so could you please put your approval on the PR? |
...on/communication/data-plane/Email/preview/2023-01-15-preview/CommunicationServicesEmail.json
Show resolved
Hide resolved
...on/communication/data-plane/Email/preview/2023-01-15-preview/CommunicationServicesEmail.json
Show resolved
Hide resolved
I took another look and approved. Added 2 comments. |
@jhendrixMSFT I think this is ready to merge. |
… preparation for GA (Azure#21841) * rename EmailMessage to Message and Sender to SenderEmail * Change EmailCustomHeaders to a dictionary of string, string * add x-ms-client-request-id header * update swagger version * Remove AttachmentType enum, change to use MIME type and update SendEmail sample * Add new version folder for updated swagger version * Restore files from 2021-10-01-preview version * Updating references and readme to point to the new preview version * Fix API version to 2023-01-15-preview instead of 2023-15-01-preview * Rename preview version folder * Revert name of Message object to EmailMessage, but retain name to be generated as message * remove retry-after header, update attachment description, change cc,bcc to lowercase * update samples based on changes so far * add x-ms-long-running-operation flag * Remove importance field from email message * update swagger to match LongRunningOperation pattern * update swagger version of CommunicationErrorResponse * Remove SendStatusResult object * rename title to EmailClient * Add more status retrieval samples * fix validation errors in swagger * update swagger based on REST API review feedback * Update casing for opreation status in all samples * update swagger based on ARB SDK Feedback * update get operation status reference schema * update property ContentBytesBase64 to ContentInBase64 * rename disableUserEngagementTracking property
Data Plane API - Pull Request
Update ACS Email swagger based on prior feedback from review board in preparation for GA. We would like to release these changes in preview before we go to GA first.
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your engagement record.
Is this review for (select one):
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous Open API document (swagger) if applicable, and the root paths that have been updated.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
Click here for links to tools, specs, guidelines & other good stuff
Tooling
Guidelines & Specifications
Helpful Links
fix #22190