Skip to content

NVAInVnet swagger changes #35093

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

Conversation

sbhosalemsft
Copy link
Contributor

Choose a PR Template

Switch to "Preview" on this description then select one of the choices below.

Click here to open a PR for a Data Plane API.

Click here to open a PR for a Control Plane (ARM) API.

Click here to open a PR for only SDK configuration.

Copy link

openapi-pipeline-app bot commented Jun 4, 2025

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ 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 Automated merging requirements met has failed. This is the final check that must pass. 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. In addition, refer to step 4 in the PR workflow diagram

Copy link

openapi-pipeline-app bot commented Jun 4, 2025

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

@github-actions github-actions bot added the brownfield Brownfield services will soon be required to convert to TypeSpec. See https://aka.ms/azsdk/typespec. label Jun 4, 2025
@AzureRestAPISpecReview AzureRestAPISpecReview added ARMReview resource-manager WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jun 4, 2025
@sbhosalemsft sbhosalemsft changed the title add the nvainvnet changes NVAInVnet swagger changes Jun 5, 2025
Copy link

Commenter does not have sufficient privileges for PR 35093 in repo Azure/azure-rest-api-specs

"type": "object",
"properties": {
"subnet": {
"readOnly": false,
Copy link
Member

Choose a reason for hiding this comment

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

type

[ARMBlockingComment] Don't use ARM top-level property names within the property bag. You could use nicType or interfaceType or something else that fits better.

Copy link
Contributor Author

@sbhosalemsft sbhosalemsft Jun 12, 2025

Choose a reason for hiding this comment

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

image

Thank you for the feedback. We understand the concern regarding the use of ARM top-level property names. However, in this case, the "type" property is already established in a related configuration(screenshot above) and is used to indicate the NIC type (PublicNic or PrivateNic). The new property being introduced is closely tied to the existing one, and both are interdependent.

To maintain consistency and parity across the API model, we would prefer to retain the name type for this property. This aligns with the current structure and avoids introducing divergence in naming conventions within the same context.

Please let us know if you have any further concerns or suggestions.

"value": "AdditionalPublicNic",
"description": "An additional public NIC type"
}
]
Copy link
Member

Choose a reason for hiding this comment

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

name

[ARMBlockingComment] Rename to something different from top-level properties (name, type, id, systemData, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Thank you for the feedback. We understand the guidance around avoiding ARM top-level property names. However, the name property is already used in the existing API for related configurations, and the new property being introduced is closely aligned with that structure (screenshot above).

To maintain consistency and parity across the API surface, especially for features that are interdependent, we would prefer to retain the use of name in this context.

Please let us know if you have any concerns with this approach.

},
"description": "Specifies input parameters required NVA in VNet interface configuration."
},
"NvaInVnetSubnetReferenceProperties": {
Copy link
Member

Choose a reason for hiding this comment

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

id

[ARMBlockingComment] Rename to something different from top-level properties (name, type, id, systemData, etc.). Recommend something the includes resourceId in the name, like resourceId or subnetResourceId, or something you like better.

Copy link
Contributor Author

@sbhosalemsft sbhosalemsft Jun 12, 2025

Choose a reason for hiding this comment

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

Thank you for the feedback @mentat9 . Referenced resources in our model are currently represented using the ResourceType.Id pattern. For example:

  1. "virtualHub": { "id": "/subscriptions/.../virtualHubs/RahulTestHub" }
  2. "internetIngressPublicIps": [{ "id": "/subscriptions/.../publicIPAddresses/slbip1" }]

To maintain alignment with this established convention—especially within Hybrid Networking—we’ve applied the same approach for the new subnet reference field. Given that the current naming is consistent with existing patterns and already integrated into the codebase, we’d prefer to retain it as is.

image
image
image
image

@mentat9 mentat9 added the ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review label Jun 11, 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 11, 2025
@sbhosalemsft sbhosalemsft requested a review from mentat9 June 12, 2025 20:44
@sbhosalemsft
Copy link
Contributor Author

/azp run

Copy link

Commenter does not have sufficient privileges for PR 35093 in repo Azure/azure-rest-api-specs

@sbhosalemsft sbhosalemsft changed the base branch from release-network-2024-10-01 to kamboj-prjwl/release-network-2024-10-01 June 16, 2025 20:49
Copy link

PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.

@sbhosalemsft sbhosalemsft changed the base branch from kamboj-prjwl/release-network-2024-10-01 to release-network-2024-10-01 June 17, 2025 01:31
@sbhosalemsft
Copy link
Contributor Author

Closing as new base branch was created by Prajwal. Will reopen PR with new base branch

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 brownfield Brownfield services will soon be required to convert to TypeSpec. See https://aka.ms/azsdk/typespec. Network resource-manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants