-
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
Adding APIs for private link #8540
Conversation
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-go
|
azure-sdk-for-java
|
azure-sdk-for-net
|
azure-sdk-for-js
|
azure-sdk-for-python
|
Can one of the admins verify this patch? |
@juniwang there is 4 validations failure to fix. you should fix avocado first then modal validation & others |
Azure Pipelines successfully started running 1 pipeline(s). |
@akning-ms Thanks for following up. Validation errors fixed. |
specification/signalr/resource-manager/Microsoft.SignalRService/stable/2018-10-01/signalr.json
Outdated
Show resolved
Hide resolved
specification/signalr/resource-manager/Microsoft.SignalRService/stable/2018-10-01/signalr.json
Outdated
Show resolved
Hide resolved
specification/signalr/resource-manager/Microsoft.SignalRService/stable/2018-10-01/signalr.json
Show resolved
Hide resolved
specification/signalr/resource-manager/Microsoft.SignalRService/stable/2018-10-01/signalr.json
Outdated
Show resolved
Hide resolved
specification/signalr/resource-manager/Microsoft.SignalRService/stable/2018-10-01/signalr.json
Show resolved
Hide resolved
@juniwang - few comments |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
azure-cli-extensions
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-trenton
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -861,7 +1083,7 @@ | |||
"readOnly": true | |||
}, | |||
"type": { | |||
"description": "The type of the service - e.g. \"Microsoft.SignalRService/SignalR\"", | |||
"description": "The type of the resource - e.g. \"Microsoft.SignalRService/SignalR\", \"Microsoft.SignalRService/SignalR/PrivateEndpointConnections\"", |
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.
Microsoft.SignalRService/SignalR/PrivateEndpointConnections" [](start = 98, length = 61)
privateEndpointConnections does not belong here as an example on this definition. This definition describes a tracked resource specific to your service. PrivateEndpointConnections are proxy
@@ -1059,7 +1297,7 @@ | |||
} | |||
}, | |||
"value": { | |||
"description": "Value of the feature flag. See Azure SignalR service document https://docs.microsoft.com/azure/azure-signalr/ for allowed values.", | |||
"description": "Value of the feature flag. See Azure SignalR service document https://docs.microsoft.com/en-us/azure/azure-signalr/ for allowed values.", |
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.
en-us/ [](start = 115, length = 6)
don't include locale in documentation links
"type": "object", | ||
"properties": { | ||
"allow": { | ||
"description": "Allowed request types. The value can be: ClientConnection, ServerConnection, RESTAPI.", |
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.
ClientConnection, ServerConnection, RESTAPI [](start = 67, length = 43)
if this is value constrained it should be represented by an array of string enums
"readOnly": true | ||
}, | ||
"type": { | ||
"description": "The type of the resource - e.g. \"Microsoft.SignalRService/SignalR\", \"Microsoft.SignalRService/SignalR/PrivateEndpointConnections\"", |
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.
"Microsoft.SignalRService/SignalR" [](start = 58, length = 36)
Please update the type description so it is valid for this definition
"description": "The type of the resource - e.g. \"Microsoft.SignalRService/SignalR\", \"Microsoft.SignalRService/SignalR/PrivateEndpointConnections\"", | ||
"type": "string", | ||
"readOnly": true | ||
} |
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.
id, name, and type are inherited from the ProxyResource->Resource reference. You don't need to redeclare them here
"type": "Microsoft.SignalRService/SignalR/privateEndpointConnections" | ||
}, | ||
"headers": { | ||
"Location": "https://management.azure.com/subscriptions/subid/providers/Microsoft.SignalRService/...pathToOperationResult..." |
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.
"Location" [](start = 8, length = 10)
Async PUT should use the azure-asyncoperation header if it needs additional operation info, not location. https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/Addendum.md#creatingupdating-using-put
}, | ||
"networkACLs": { | ||
"$ref": "#/definitions/SignalRNetworkACLs", | ||
"description": "Network ACLs" | ||
} |
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.
this needs to be a new api-version. Handling it via "patch" semantics with nulls is not a good approach as that is not expected by consumers of your API (a PUT should replace all properties). It makes what-if analysis on template deployments much more complex, will break users with custom Azure Policies in place that require certain properties, etc...
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.
🕐
Thanks for review, we will have a internal discussion first about the api-version |
Close this PR since we have another one: #9377 |
Changes
upstream
For both changes, backward compatibilities are took into account. If they are
null
, whether it's from old SDK or newer version of SDK, we won't change anything. Only if it's notnull
, we read their children properties and handle it correspondingly.Validations documented at Swagger Validation tools all passed.
Latest improvements:
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.