Skip to content

Conversation

@ericasp16
Copy link
Member

@ericasp16 ericasp16 commented Oct 18, 2023

Data Plane API - Pull Request

Updates prior to the GA of Number Lookup feature. Customers can search any phone number to receive formatting and operator information details about the searched number.
This pull request represents the preferred design by the feature team. Alternative designs to consider can be found at the links below:

API Info: The Basics

Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.

Is this review for (select one):

  • a private preview
  • a public preview
  • GA release

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 API Spec document (if applicable), and the root paths that have been updated.

Viewing API changes

For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.

Suppressing failures

If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
Swagger-Suppression-Process
to get approval.

❔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

@ericasp16 ericasp16 requested a review from a team as a code owner October 18, 2023 22:19
@ericasp16 ericasp16 requested review from johanste and weidongxu-microsoft and removed request for a team October 18, 2023 22:19
@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Oct 18, 2023

Next Steps to Merge

✔️ All automated merging requirements have been met! Refer to step 4 in the PR workflow diagram (even if your PR is for data plane, not ARM).

@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Oct 18, 2023

Swagger Validation Report

️️✔️BreakingChange succeeded [Detail] [Expand]
There are no breaking changes.
️⚠️Breaking Change(Cross-Version): 4 Warnings warning [Detail]
compared swaggers (via Oad v0.10.4)] new version base version
phonenumbers.json 2024-03-01(e91cb6e) 2022-12-01(main)
phonenumbers.json 2024-03-01(e91cb6e) 2023-05-01-preview(main)

The following breaking changes are detected by comparison with the latest preview version:

Rule Message
⚠️ 1019 - RemovedEnumValue The new version is removing enum value(s) 'unknown' from the old version.
New: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L1528:9
Old: PhoneNumbers/preview/2023-05-01-preview/phonenumbers.json#L1504:9
⚠️ 1025 - RequiredStatusChange The 'required' status changed from the old version('False') to the new version('True').
New: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L1515:7
Old: PhoneNumbers/preview/2023-05-01-preview/phonenumbers.json#L1499:7
⚠️ 1034 - AddedRequiredProperty The new version has new required property 'phoneNumber' that was not found in the old version.
New: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L1503:11
Old: PhoneNumbers/preview/2023-05-01-preview/phonenumbers.json#L1490:11
⚠️ 1034 - AddedRequiredProperty The new version has new required property 'phoneNumber' that was not found in the old version.
New: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L1509:5
Old: PhoneNumbers/preview/2023-05-01-preview/phonenumbers.json#L1496:5
️⚠️LintDiff: 3 Warnings warning [Detail]
compared tags (via openapi-validator v2.1.6) new version base version
package-phonenumber-2024-03-01 package-phonenumber-2024-03-01(e91cb6e) default(ericasp/number-lookup-ga-staging)

[must fix]The following errors/warnings are introduced by current PR:

Rule Message Related RPC [For API reviewers]
⚠️ PaginationResponse Operation might be pageable. Consider adding the x-ms-pageable extension.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L849
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L882
⚠️ EnumInsteadOfBoolean 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: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L1490


The following errors/warnings exist before current PR submission:

Only 30 items are listed, please refer to log for more details.

Rule Message
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L20
⚠️ PaginationResponse Response body schema of pageable response should contain top-level array property value
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L97
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L101
⚠️ PaginationResponse Response body schema of pageable response should contain top-level array property value
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L159
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L163
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L192
⚠️ PaginationResponse Response body schema of pageable response should contain top-level array property value
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L234
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L238
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L267
⚠️ PaginationResponse Response body schema of pageable response should contain top-level array property value
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L331
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L335
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L367
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L412
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L441
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L459
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L515
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L544
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L568
⚠️ DeleteInOperationName 'DELETE' operation 'PhoneNumbers_CancelOperation' should use method name 'Delete'. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L586
⚠️ OperationId OperationId for delete method should contain 'Delete'
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L586
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L588
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L603
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L634
⚠️ RequestBodyOptional The body parameter is not marked as required.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L644
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L678
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L707
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L725
⚠️ DeleteInOperationName 'DELETE' operation 'PhoneNumbers_ReleasePhoneNumber' should use method name 'Delete'. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change.
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L743
⚠️ OperationId OperationId for delete method should contain 'Delete'
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L743
⚠️ PathParameterSchema Path parameter should specify a maximum length (maxLength) and characters allowed (pattern).
Location: PhoneNumbers/stable/2024-03-01/phonenumbers.json#L745
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Oct 18, 2023

Swagger Generation Artifacts

️️✔️ApiDocPreview succeeded [Detail] [Expand]
 Please click here to preview with your @microsoft account. 
️️✔️SDK Breaking Change Tracking succeeded [Detail] [Expand]

Breaking Changes Tracking

Posted by Swagger Pipeline | How to fix these errors?

@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Oct 18, 2023

Generated ApiView

Language Package Name ApiView Link
Swagger communication-data-plane-PhoneNumbers https://apiview.dev/Assemblies/Review/71f3ac4060cb4d58a27f7cddf0f94012?revisionId=9ee040865f2a4d88b30c04899c9d5a33

@openapi-pipeline-app
Copy link

Swagger Validation Report

️⌛BreakingChange pending [Detail]
️⌛Breaking Change(Cross-Version) pending [Detail]
️⌛CredScan pending [Detail]
️🔄LintDiff inProgress [Detail]
️⌛Avocado pending [Detail]
️⌛SwaggerAPIView pending [Detail]
️⌛TypeSpecAPIView pending [Detail]
️⌛ModelValidation pending [Detail]
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️⌛PoliCheck pending [Detail]
️🔄PrettierCheck inProgress [Detail]
️🔄SpellCheck inProgress [Detail]
️⌛Lint(RPaaS) pending [Detail]
️⌛PR Summary pending [Detail]
️⌛Automated merging requirements met pending [Detail]
Posted by Swagger Pipeline | How to fix these errors?

@openapi-pipeline-app
Copy link

Swagger Validation Report

️🔄BreakingChange inProgress [Detail]
️🔄Breaking Change(Cross-Version) inProgress [Detail]
️⌛CredScan pending [Detail]
️🔄LintDiff inProgress [Detail]
️⌛Avocado pending [Detail]
️🔄SwaggerAPIView inProgress [Detail]
️⌛TypeSpecAPIView pending [Detail]
️⌛ModelValidation pending [Detail]
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️⌛PoliCheck pending [Detail]
️🔄PrettierCheck inProgress [Detail]
️🔄SpellCheck inProgress [Detail]
️⌛Lint(RPaaS) pending [Detail]
️⌛PR Summary pending [Detail]
️⌛Automated merging requirements met pending [Detail]
Posted by Swagger Pipeline | How to fix these errors?

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Nov 9, 2023

Do we have a review meeting schedule?

guoqing2023 and others added 2 commits November 14, 2023 09:29
Preferred option: with query parameter "expand=OperatorDetails", response will also include "isoCountryCode, operatorDetails, numberType"
@tjprescott
Copy link
Member

Has this been reviewed by the API Stewardship board?

@ericasp16
Copy link
Member Author

This was reviewed by the board, we received approval on the design pending follow-up on some potential name changes. I will be updating this review to check into a feature branch until we are ready to ship GA, probably in Q1 of next year

Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

Awaiting switch from targeting main to targeting feature branch.

@ericasp16 ericasp16 changed the base branch from main to ericasp/number-lookup-ga-staging December 13, 2023 00:27
@tjprescott
Copy link
Member

@ericasp16 are you ready for this to be merged?

@ericasp16
Copy link
Member Author

@ericasp16 are you ready for this to be merged?

yes

@tjprescott tjprescott merged commit 26919d6 into Azure:ericasp/number-lookup-ga-staging Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants