Skip to content

Convert Notificationhubs Swagger to Tsp #34320

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

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

mcgallan
Copy link
Member

@mcgallan mcgallan commented Apr 29, 2025

This PR migrates your latest version (identified by the tag in your readme.md) of swagger to TypeSpec. We already tried our best to make sure the TypeSpec represents same as previous swagger. Since we lack the business knowledge, please validate this PR again to make sure it's functional equivalent as before. The local validation step is at Getting started | TypeSpec Azure

Besides, TypeSpec encourages to follow ARM guidelines. Therefore, some representations in your previous swagger will be fixed to follow these guidelines. When you see differences in your local validation, please keep this note in mind.

Please reach out to TypeSpec Discussions Channel if there is any help needed.

Copy link

openapi-pipeline-app bot commented Apr 29, 2025

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ This PR targets either the main branch of the public specs repo or the RPSaaSMaster branch of the private specs repo. These branches are not intended for iterative development. Therefore, you must acknowledge you understand that after this PR is merged, the APIs are considered shipped to Azure customers. Any further attempts at in-place modifications to the APIs will be subject to Azure's versioning and breaking change policies. Additionally, for control plane APIs, you must acknowledge that you are following all the best practices documented by ARM at aka.ms/armapibestpractices. If you do intend to release the APIs to your customers by merging this PR, add the PublishToCustomers label to your PR in acknowledgement of the above. Otherwise, retarget this PR onto a feature branch, i.e. with prefix release- (see aka.ms/azsdk/api-versions#release--branches).
  • ❌ This PR is in purview of the ARM review (label: ARMReview). This PR must get ARMSignedOff label from an ARM reviewer.
    This PR has ARMChangesRequested label. Please address or respond to feedback from the ARM API reviewer.
    When you are ready to continue the ARM API review, please remove the ARMChangesRequested label.
    Automation should then add WaitForARMFeedback label.
    ❗If you don't have permissions to remove the label, request write access per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositories.
    For details of the ARM review, see aka.ms/azsdk/pr-arm-review
  • ❌ The required check named Swagger LintDiff has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

Copy link

openapi-pipeline-app bot commented Apr 29, 2025

PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

Copy link

github-actions bot commented Apr 29, 2025

API Change Check

APIView identified API level changes in this PR and created the following API reviews

Language API Review for Package
Swagger Microsoft.NotificationHubs
TypeSpec Microsoft.NotificationHubs
Java com.azure.resourcemanager:azure-resourcemanager-notificationhubs
Python azure-mgmt-notificationhubs

@pshao25 pshao25 marked this pull request as ready for review May 7, 2025 08:58
@pshao25 pshao25 added Versioning-Approved-Benign https://github.com/Azure/azure-sdk-tools/issues/6374 and removed NotReadyForARMReview labels Jun 19, 2025
@AzureRestAPISpecReview AzureRestAPISpecReview added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jun 20, 2025
*/
@visibility(Lifecycle.Read)
// FIXME: (utcDateTime) Please double check that this is the correct type for your scenario.
createdTime?: utcDateTime;
Copy link
Member

Choose a reason for hiding this comment

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

what was the original documented format?

*/
@visibility(Lifecycle.Read)
privateLinkServiceArmRegion?: string;
}
Copy link
Member

@TimLovellSmith TimLovellSmith Jun 24, 2025

Choose a reason for hiding this comment

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

[ARMBlockingComment] possible antipattern documenting internal private link contracts as if external?

This comment suggests its an internal API contract, in which case it shouldn't even be documented here should it?

/**

  • Represents a connectivity information to Notification Hubs namespace. This is part of PrivateLinkService proxy that tell
  • the Networking RP how to connect to the Notification Hubs namespace.
    */


/**
* A customer-visible sub-resource of Private Endpoint, which describe the connection between Private Endpoint and Notification Hubs namespace.
*/
Copy link
Member

Choose a reason for hiding this comment

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

customer-visible ? that should go without saying, in the public API doc!

}

/**
* Result of the request to list SQL operations.
Copy link
Member

Choose a reason for hiding this comment

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

SQL

SQL?

/**
* Result of the request to list SQL operations.
*/
model Operation {
Copy link
Member

Choose a reason for hiding this comment

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

Operation

[ARMBlockingComment] why not use common types Operation definitions?

model OperationListResult is Azure.Core.Page<Operation>;

#suppress "@azure-tools/typespec-azure-core/documentation-required" "FIXME: Update justification, follow aka.ms/tsp/conversion-fix for details"
model OperationDisplay {
Copy link
Member

Choose a reason for hiding this comment

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

OperationDisplay

[ARMBlockingComment] why not use common types Operation definitions?

* ARM resource ID of the Private Endpoint. This may belong to different subscription and resource group than a Notification Hubs namespace.
*/
@visibility(Lifecycle.Read)
id?: string;
Copy link
Member

Choose a reason for hiding this comment

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

string

should be annoted as a resource id right, something like x-ms-resource-id as we have in swagger?

/**
* Private Endpoint Connection properties.
*/
model PrivateEndpointConnectionProperties {
Copy link
Member

Choose a reason for hiding this comment

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

PrivateEndpointConnectionProperties

why not in the same typespec doc (TSP file) as PrivateEndpointConnectionResource ?

* Represents a Private Endpoint Connection ARM resource - a sub-resource of Notification Hubs namespace.
*/
@parentResource(NamespaceResource)
model PrivateEndpointConnectionResource
Copy link
Member

Choose a reason for hiding this comment

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

PrivateEndpointConnectionResource

Putting 'Resource' on the name is more of a naming antipattern I think - were we already doing it?


namespace Microsoft.NotificationHubs;
/**
* A Private Link Arm Resource.
Copy link
Member

Choose a reason for hiding this comment

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

Arm

avoid 'Arm' terminiology in favor of 'Azure', 'Azure resource', etc

@armResourceOperations
interface PrivateLinkResources {
/**
* Even though this namespace requires subscription id, resource group and namespace name, it returns a constant payload (for a given namespacE) every time it's called.
Copy link
Member

Choose a reason for hiding this comment

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

namespacE

caplitalization: namespace

getGroupId is ArmResourceRead<PrivateLinkResource>;

/**
* Even though this namespace requires subscription id, resource group and namespace name, it returns a constant payload (for a given namespacE) every time it's called.
Copy link
Member

Choose a reason for hiding this comment

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

namespacE

caplitalization: namespace

@TimLovellSmith
Copy link
Member

      "format": "password",

Whats a SID? Are sids or package ids actually secrets? I'm a little confused about this, and just want some explanation or sanity check


Refers to: specification/notificationhubs/resource-manager/Microsoft.NotificationHubs/preview/2023-10-01-preview/notificationhubs.json:4028 in 1273ec6. [](commit_id = 1273ec6, deletion_comment = False)

@TimLovellSmith TimLovellSmith added the ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review label Jun 24, 2025
@openapi-pipeline-app openapi-pipeline-app bot removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jun 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review ARMReview Notification Hub resource-manager TypeSpec Authored with TypeSpec typespec-conversion-w1 Versioning-Approved-Benign https://github.com/Azure/azure-sdk-tools/issues/6374 VersioningReviewRequired <valid label in PR review process>add this label when versioning review is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants