add service fabric swagger file#984
Conversation
|
@chingchenmsft, |
| "tags": [ | ||
| "ClusterpatchOperation" | ||
| ], | ||
| "operationId": "ClusterOperation_Update", |
There was a problem hiding this comment.
The first part of the operation id (before _ ) should be a plural, also it is recommended to match this part with URL collection segment. If you apply both rules this can be named as "Clusters_Update"
There was a problem hiding this comment.
Thanks Anu for your time, so the operation is against one cluster a time, should we still use "Clusters" not "Cluster"?
There was a problem hiding this comment.
@chingchenmsft The first part becomes the name of the generated container class for the operations, something like:
class Clusters {
Cluster Get(..)
Cluster Create(..)
Cluster Update(..)
List<Cluster> List()
..
}This is one of the patterns we recommend to have consistently in naming across RPs.
| } | ||
| } | ||
| }, | ||
| "ClusterResource": { |
There was a problem hiding this comment.
Remove 'Resource' from the name, cluster is already a resource (allOf Resource).
| "tags": [ | ||
| "ClusterGetOperation" | ||
| ], | ||
| "operationId": "ClusterOperation_get", |
There was a problem hiding this comment.
ClusterOperation_get -> Clusters_Get
| "tags": [ | ||
| "ClusterCreateOperation" | ||
| ], | ||
| "operationId": "ClusterOperation_Create", |
There was a problem hiding this comment.
ClusterOperation_Create -> Clusters_Create
| } | ||
| } | ||
| }, | ||
| "paasClusterUpgradePolicy": { |
| } | ||
| } | ||
| }, | ||
| "diagnosticsStorageAccountConfig": { |
There was a problem hiding this comment.
make sure all model names are PascalCase. diagnosticsStorageAccountConfig -> DiagnosticsStorageAccountConfig
| } | ||
| } | ||
| }, | ||
| "clusterResourceProperties": { |
There was a problem hiding this comment.
PascalCase, clusterResourceProperties -> ClusterResourceProperties
| "clusterId": { | ||
| "type": "string" | ||
| }, | ||
| "clusterState": { |
There was a problem hiding this comment.
"clusterState" is a property name, should we make the first letter as lower case ?
There was a problem hiding this comment.
sorry, please disregard this comment
| } | ||
| } | ||
| }, | ||
| "paths": { |
There was a problem hiding this comment.
Does RP supports Listing resources (clusters) by subscription and resource group? if yes please add the same operations
Clusters_List -> Lists all of the clusters within an Azure subscription
Clusters_ListByResourceGroup -> Lists all the Clusters under a resource group.
There was a problem hiding this comment.
Does RP support deleting cluster? if so, please add the operation.
| "$ref": "#/definitions/ErrorModel" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this a long running operation (LRO)? if yes please mark it so using x-ms-long-running-operation https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/swagger-extensions.md#x-ms-long-running-operation
There was a problem hiding this comment.
it is not long running, all our API here are NOT long running API.
There was a problem hiding this comment.
same comment as above for create - (1) response model for 200 (2) removing 202
There was a problem hiding this comment.
i add it back, because we will make the operation as long running shortly
| "schema": { | ||
| "$ref": "#/definitions/ErrorModel" | ||
| } | ||
| } |
There was a problem hiding this comment.
Is Create a long running operation (LRO)? if yes please mark it so using x-ms-long-running-operation https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/swagger-extensions.md#x-ms-long-running-operation
There was a problem hiding this comment.
it is not long running, all our API here are NOT long running API.
There was a problem hiding this comment.
Ok, can we define response model for 200? I assume it is Cluster
"200": {
"description": "OK, Cluster successfully created.",
"schema": {
"$ref": "#/definitions/<ResponseModel>"
}
},If create is not LRO then we can remove 202 right?
There was a problem hiding this comment.
i add it back, because we will make the operation as long running shortly
| "properties": { | ||
| "$ref": "#/definitions/clusterResourceProperties" | ||
| } | ||
| }, |
There was a problem hiding this comment.
By default, we flatten 'properties' by one level. Here in this case
ClusterResource: {
"properties": {
"firstProperty": {
"type": "string"
},
"properties": {
"$ref": "#/definitions/clusterResourceProperties"
}
}
}The generated class will be used as:
ClusterResource rs;
rs.myProperty = "";
rs.properties.clusterId = "";But if you apply x-ms-client-flatten extension https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/swagger-extensions.md#x-ms-client-flatten as below:
ClusterResource: {
"properties": {
"firstProperty": {
"type": "string"
},
"properties": {
"x-ms-client-flatten": true,
"$ref": "#/definitions/clusterResourceProperties"
}
}
}then generated type will be used as:
ClusterResource rs;
rs.myProperty = "";
rs.clusterId = "";We recommend using this extension so that end users experience will be better. I guess this is one of the warning you are getting from validator.
There was a problem hiding this comment.
Yes, but we can't do this, because the existing contract from ARM define in this way.
There was a problem hiding this comment.
as mentioned in the previous comment, This flattening is only applied to the generated type, each language specific runtime will ensure payload on the wire conforms to the contract.
There was a problem hiding this comment.
@amarzavery could you please chime in? My understanding is "x-ms-client-flatten" affects only the generated model but when the model is serialized the resulting json will contain nested "properties"
but @chingchenmsft mentioned this is not the case. When he tried .NET SDK generated from a swagger (with this extension applied) ARM failed the request.
There was a problem hiding this comment.
Yup the extension only applies to how the model will be presented in the sdk. It does not affect the wire contract.
There is one caveat for applying this extension: If the model is polymorphic then make sure that flattening does not cause issues.
There was a problem hiding this comment.
thanks, yes you are right, i changed to use this flag, not sure why last time i failed to do this:)
| "$ref": "#/definitions/clusterPatchParameters" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Do you think we can provide a better name for the type ClusterPatchRequest ? with this name the prototype of the update method will looks like
class Clusters {
Cluster Update(String resourceGroupName, String customerName, ClusterPatchRequest clusterPatchRequest);
}How about re-naming ClusterPatchRequest to ClusterUpdateParameters ? and the nested ClusterPatchParameters can be named as ClusterPropertiesUpdateParameters. This name seems better match with the operation name (Clusters.Update).
In addition to this, we could apply x-ms-client-flatten extension https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/swagger-extensions.md#x-ms-client-flatten here as well for simple user experience (Ref: https://github.com/Azure/azure-rest-api-specs/pull/984/files#r104088396)
There was a problem hiding this comment.
i have renamed it, but we can't flatten it, because doing that will fail our request, ARM doesn't allow do this.
There was a problem hiding this comment.
This flattening is only applied to the generated type, each language specific runtime will ensure payload on the wire looks as expected by RP.
There was a problem hiding this comment.
Did not quite fellow, last time i tried to do this, and ARM failed my request, not sure what i have missed when generated .net SDK library.
| "$ref": "#/definitions/ErrorModel" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think "response code 202" and "default" are not applicable for List operation, Right?
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same comment as above for "response code 202" and "default"
| "description": "Accepted", | ||
| "schema": { | ||
| "$ref": "#/definitions/Cluster" | ||
| } |
| "responses": { | ||
| "200": { | ||
| "description": "OK" | ||
| }, |
There was a problem hiding this comment.
One of the required response code for DELETE is "204", which is missing.
As per PR guideline for DELETE: "The resource provider can return 200 (OK) or 204 (NoContent) to indicate that the operation completed successfully. A 200 (OK) should be returned if the object exists and was deleted successfully; and a 204 (NoContent) should be used if the resource does not exist and the request is well formed."
There was a problem hiding this comment.
@chingchenmsft Please provide x-ms-examples of each operation as well.
chingchenmsft
left a comment
chingchenmsft
left a comment
There was a problem hiding this comment.
added example
| "schema": { | ||
| "$ref": "#/definitions/ClusterUpdateParameters" | ||
| }, | ||
| "description": "Patch Request" |
There was a problem hiding this comment.
can we have a detailed description for e.g. "The parameters to provide for the updated cluster."
| } | ||
| }, | ||
| "202": { | ||
| "description": "Accept" |
There was a problem hiding this comment.
detailed description e.g. "Accepted - Update request accepted; the operation will complete asynchronously."
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", |
There was a problem hiding this comment.
detailed description e.g. "OK - Cluster updated successfully"
| "readOnly": true | ||
| }, | ||
| "diagnosticsStorageAccountConfig": { | ||
| "$ref": "#/definitions/DiagnosticsStorageAccountConfig" |
There was a problem hiding this comment.
As per our offline discussion, this property 'diagnosticsStorageAccountConfig' is read-only. Can we mark as read-only here and remove read-only applied to individual properties in the model "definitions/DiagnosticsStorageAccountConfig"?
There was a problem hiding this comment.
can be changed
| "properties": { | ||
| "name": { | ||
| "type": "string", | ||
| "description": "name" |
There was a problem hiding this comment.
can we have sufficient description for 'name' property?
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/definitions/ClientCertificateThumbprint" | ||
| } |
There was a problem hiding this comment.
Is 'clientCertificateThumbprints' settable through PUT/PATCH? or is it falls under properties that can be set only via template? checking if this is eligible to mark read-only
There was a problem hiding this comment.
clientCertificateThumbprints is ok to be changed.
There was a problem hiding this comment.
Suppose user wants to add a ClientCertificateThumbprint instance to this collection via PATCH, which one of the following flow is correct?
Flow1
- clusterPropertiesUpdateParameters = new ClusterPropertiesUpdateParameters();
- User gets the
clusterinstance - set clusterPropertiesUpdateParameters.clientCertificateThumbprints = cluster.clientCertificateThumbprints;
- clusterPropertiesUpdateParameters.clientCertificateThumbprints.add(newClientCertificateThumbprint);
- PATCH clusterPropertiesUpdateParameters
Flow2:
- clusterPropertiesUpdateParameters = new ClusterPropertiesUpdateParameters();
- clusterPropertiesUpdateParameters.clientCertificateThumbprints.add(newClientCertificateThumbprint);
- PATCH clusterPropertiesUpdateParameters
Asking to confirm, incase of PATCH - whether sending an array with new items appends that to existing array in the server or it overwrites the previous collection.
It will be helpful to indicate this in the description, i.e. in general how the array needs to be PATCH-ed
There was a problem hiding this comment.
it will overwrite.
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/definitions/ClientCertificateCommonName" | ||
| } |
There was a problem hiding this comment.
confirming - is 'clientCertificateCommonNames' eligible to mark as read-only?
There was a problem hiding this comment.
clientCertificateCommonNames is ok to be changed.
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/definitions/SettingsSectionDescription" | ||
| } |
There was a problem hiding this comment.
confirming - is 'fabricSettings' eligible to mark as read-only?
There was a problem hiding this comment.
fabricSettings is ok to be changed,
There was a problem hiding this comment.
Based on your previous comment, I think overwrite is the behavior for this array as well. If possible please provide a meaningful description for this property and indicate PATCH behavior.
There was a problem hiding this comment.
done, but this one (fabricSettings) is not be override.
| } | ||
| }, | ||
| "reverseProxyCertificate": { | ||
| "$ref": "#/definitions/CertificateDescription" |
There was a problem hiding this comment.
confirming - is 'reverseProxyCertificate' eligible to mark as read-only?
There was a problem hiding this comment.
reverseProxyCertificate is ok to be changed, since it is just a setting in service fabric .
There was a problem hiding this comment.
same comment as above for description
| "$ref": "#/definitions/DiagnosticsStorageAccountConfig" | ||
| }, | ||
| "upgradeDescription": { | ||
| "$ref": "#/definitions/PaasClusterUpgradePolicy" |
There was a problem hiding this comment.
confirming - is upgradeDescription read-only ?
There was a problem hiding this comment.
upgradeDescription is ok to be changed.
| "api-version": "2016-09-01", | ||
| "ClusterResource": { | ||
| "properties": { | ||
| "clusterId": "4d0af3c4-04c4-4ea4-a7a0-5917aca0fe03", |
There was a problem hiding this comment.
does not look like input.
There was a problem hiding this comment.
clusterId, clusterEndpoint, clusterCodeVersion, are these settable?
There was a problem hiding this comment.
clusterCodeVersion yes, i will remove the other two.
There was a problem hiding this comment.
user can input, but we just ignore them for the other two.
| } | ||
| } | ||
| }, | ||
| "id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/resRg/providers/Microsoft.ServiceFabric/clusters/myCluster", |
There was a problem hiding this comment.
only location and tag are input there.
There was a problem hiding this comment.
removed others.
anudeepsharma
left a comment
anudeepsharma
left a comment
There was a problem hiding this comment.
Requesting changes.
| "clusterEndpoint": { | ||
| "type": "string" | ||
| }, | ||
| "clusterCodeVersion": { |
There was a problem hiding this comment.
How user will know code version value, describe in description
| "$ref": "#/definitions/ClusterVersionDetails" | ||
| } | ||
| }, | ||
| "clusterId": { |
| "name": "Security", | ||
| "parameters": [ | ||
| { | ||
| "name": "ClusterProtectionLevel", |
There was a problem hiding this comment.
I think name - value here should be enums, how user will know what values to send in.
These does not look like tags,
| "clientCertificateThumbprints": [], | ||
| "clientCertificateCommonNames": [], | ||
| "fabricSettings": [], | ||
| "managementEndpoint": "https://myCluster.southcentralus.cloudapp.azure.com:19080", |
There was a problem hiding this comment.
Does not look like settable
There was a problem hiding this comment.
as offline chat, it can be changed.
| "reverseProxyCertificate": { | ||
| "$ref": "#/definitions/CertificateDescription" | ||
| }, | ||
| "managementEndpoint": { |
There was a problem hiding this comment.
offline chat, it can be changed.
| "certificate": {}, | ||
| "reliabilityLevel": "Silver", | ||
| "upgradeMode": "Automatic", | ||
| "clientCertificateThumbprints": [], |
There was a problem hiding this comment.
do we need to send empty arrays
| "description": "Resource tags." | ||
| } | ||
| }, | ||
| "required": [ |
There was a problem hiding this comment.
missing required properties.
look at https://github.com/Azure/azure-rest-api-specs/blob/master/arm-compute/2016-04-30-preview/swagger/disk.json#L811
There was a problem hiding this comment.
are you looking at wrong place, this is the ARM "Resource": {} , i think location is the only required.
| "Canceled" | ||
| ] | ||
| }, | ||
| "vmImage": { |
There was a problem hiding this comment.
please add description for all the fields.
| "vmInstanceCount": 5 | ||
| } | ||
| ], | ||
| "provisioningState": "Succeeded", |
There was a problem hiding this comment.
ProvisioningState can't be updated by user.
There was a problem hiding this comment.
changed to read only
| ], | ||
| "provisioningState": "Succeeded", | ||
| "vmImage": "Windows", | ||
| "diagnosticsStorageAccountConfig": { |
There was a problem hiding this comment.
is diagnostic settings required for resource?
| "type": "boolean", | ||
| "description": "Is primary or not" | ||
| }, | ||
| "vmInstanceCount": { |
There was a problem hiding this comment.
Put description for all the fields.
| "name": "Security", | ||
| "parameters": [ | ||
| { | ||
| "name": "ClusterProtectionLevel", |
There was a problem hiding this comment.
Can these parameters hold any string, or are these candidate for enum
There was a problem hiding this comment.
This is string in our service.
There was a problem hiding this comment.
So I am assuming your service accepts any string for this.
There was a problem hiding this comment.
yeah, all are string for fabricSettings
| "required": [ | ||
| "managementEndpoint", | ||
| "reliabilityLevel", | ||
| "provisioningState", |
There was a problem hiding this comment.
provisioningState can't be a required parameter. Why you will ask customer to provide this?
| "description": "The provisioning state of the cluster", | ||
| "type": "string", | ||
| "enum": [ | ||
| "Default", |
There was a problem hiding this comment.
Creating is missing, what is meant by Default here, do you send default state out.
| } | ||
| } | ||
| }, | ||
| "DiagnosticsStorageAccountConfig": { |
There was a problem hiding this comment.
If this is required, I am assuming, fields with in this is also required, like primaryAccessKey etc, in case properties in this class are not marked required, SDK will not validate the properties on client side and call will reach service just to be failed.
| "description": "Certificate description", | ||
| "properties": { | ||
| "thumbprint": { | ||
| "type": "string" |
There was a problem hiding this comment.
Provide a description e.g. "hexadecimal string that uniquely identifies a certificate."
| "type": "string" | ||
| }, | ||
| "x509StoreName": { | ||
| "type": "string", |
There was a problem hiding this comment.
Provide a description r.g. "name of the X.509 certificate store"
| "type": "string" | ||
| }, | ||
| "thumbprintSecondary": { | ||
| "type": "string" |
| } | ||
| }, | ||
| "SettingsSectionDescription": { | ||
| "description": "Settings section description", |
There was a problem hiding this comment.
Better description please - e.g. "Type representing a collection of settings applied to the fabric cluster"
| "properties": { | ||
| "name": { | ||
| "type": "string", | ||
| "description": "The name of settings section description" |
There was a problem hiding this comment.
Better description - "unique name for the settings collection".
@chingchenmsft - fabricSettings is a collection of SettingsSectionDescription instances. Can we assume the name is unique?
| ] | ||
| }, | ||
| "applicationPorts": { | ||
| "description": "Application ports", |
There was a problem hiding this comment.
Better description please - "The port (endpoint) range allocated for applications deployed in the cluster"
There was a problem hiding this comment.
If this is the range of public ports used by the deployed applications to listen for incoming traffic from the Internet, then please add that info to description
| "$ref": "#/definitions/EndpointRangeDescription" | ||
| }, | ||
| "ephemeralPorts": { | ||
| "description": "Ephemeral ports", |
There was a problem hiding this comment.
Can we have better description here please? which clarifies the word Ephemeral
| } | ||
| }, | ||
| "EndpointRangeDescription": { | ||
| "description": "Endpoint range description", |
There was a problem hiding this comment.
better description - "Type representing an endpoint range."
| "description": "Endpoint range description", | ||
| "properties": { | ||
| "startPort": { | ||
| "type": "integer" |
| "type": "integer" | ||
| }, | ||
| "endPort": { | ||
| "type": "integer" |
|
No modification for Ruby |
|
No modification for NodeJS |
|
No modification for Python |
9 participants
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-versionin the path should match theapi-versionin the spec).Quality of Swagger