Skip to content
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

Added CheckNameAvailability, Identity, systemData, used ErrorResponse v2 in DeviceUpdate #13750

Merged
merged 18 commits into from
Apr 9, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,52 @@
}
},
"paths": {
"/subscriptions/{subscriptionId}/providers/Microsoft.DeviceUpdate/checknameavailability": {
"post": {
"description": "Checks ADU resource name availability.",
"operationId": "CheckNameAvailability",
"parameters": [
{
"$ref": "#/parameters/ApiVersionParameter"
},
{
"$ref": "#/parameters/SubscriptionIdParameter"
},
{
"schema": {
"$ref": "../../../../../common-types/resource-management/v2/types.json#/definitions/CheckNameAvailabilityRequest"
},
"in": "body",
"name": "request",
"required": true,
"description": "Check Name Availability Request."
}
],
"responses": {
"200": {
"description": "Check Name Availability Response.",
"schema": {
"$ref": "../../../../../common-types/resource-management/v2/types.json#/definitions/CheckNameAvailabilityResponse"
}
},
"default": {
"description": "Error response describing the reason for operation failure.",
"schema": {
"$ref": "#/definitions/ErrorDefinition"
}
}
},
"x-ms-examples": {
"CheckNameAvailability_Available": {
"$ref": "./examples/CheckNameAvailability_Available.json"
},
"CheckNameAvailability_AlreadyExists": {
"$ref": "./examples/CheckNameAvailability_AlreadyExists.json"
}
},
"deprecated": false
}
},
"/subscriptions/{subscriptionId}/providers/Microsoft.DeviceUpdate/accounts": {
"get": {
"description": "Returns list of Accounts.",
Expand Down Expand Up @@ -222,7 +268,7 @@
],
"responses": {
"200": {
"description": "Async operation to delete Account was created."
"description": "Account was deleted."
},
"202": {
"description": "Async operation to delete Account was created."
Expand Down Expand Up @@ -271,7 +317,13 @@
],
"responses": {
"200": {
"description": "Account updated successfully.",
"description": "Account was updated successfully.",
"schema": {
"$ref": "#/definitions/Account"
}
},
"201": {
Copy link

@varunkumta varunkumta Apr 6, 2021

Choose a reason for hiding this comment

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

201 [](start = 11, length = 3)

a patch should not return a 201, but a 202 to indicate async completion - https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/Addendum.md#updating-using-patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually PUT returns 201 and DELETE returns 202. In our case PATCH initiates the same flow as PUT hence 201.

Choose a reason for hiding this comment

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

201 is Created, a PATCH cannot be used to create a resource - and hence it is not a valid status code to return

Copy link
Contributor

Choose a reason for hiding this comment

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

We are behind RPaaS and currently they do not support 202 for async operation only 201 even for patch. We reached out to them and they said they will work either to introduce 202 or give guidance to accept 201 for there reviews in the future. For now here is the documentation https://armwiki.azurewebsites.net/rpaas/async.html If there are any questions please reach out to RPaaS Team.

Choose a reason for hiding this comment

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

I confirmed with the RPaaS team that this is an issue today, so I will sign off. However they are looking at changing it so please work with them as to what they want you to do for this PR

"description": "Account was updated successfully.",
"schema": {
"$ref": "#/definitions/Account"
}
Expand Down Expand Up @@ -456,7 +508,7 @@
],
"responses": {
"200": {
"description": "Async operation to delete Instance was created."
"description": "Instance was deleted."
},
"202": {
"description": "Async operation to delete Instance was created."
Expand Down Expand Up @@ -508,7 +560,7 @@
],
"responses": {
"200": {
"description": "Instance updated successfully.",
"description": "Instance was updated successfully.",
"schema": {
"$ref": "#/definitions/Instance"
}
Expand Down Expand Up @@ -567,6 +619,10 @@
}
],
"properties": {
"systemData": {
"$ref": "../../../../../common-types/resource-management/v1/types.json#/definitions/systemData",
"readOnly": true
},
"properties": {
"description": "Device Update account properties.",
"x-ms-client-flatten": true,
Expand Down Expand Up @@ -595,6 +651,10 @@
"readOnly": true
}
}
},
"identity": {
"$ref": "#/definitions/Identity",
"description": "The type of identity used for the resource."
}
}
},
Expand Down Expand Up @@ -624,6 +684,10 @@
}
],
"properties": {
"systemData": {
"$ref": "../../../../../common-types/resource-management/v1/types.json#/definitions/systemData",
"readOnly": true
},
"properties": {
"description": "Device Update instance properties.",
"x-ms-client-flatten": true,
Expand Down Expand Up @@ -682,6 +746,33 @@
}
}
},
"Identity": {

Choose a reason for hiding this comment

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

this is available in common-types, any reason to not re-use that?

Copy link
Contributor

Choose a reason for hiding this comment

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

The common type includes identity type enums we do not support at the moment like "UserAssigned" and "SystemAssigned, UserAssigned"

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think the common definition did no include "None" which is a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both common types v1 and v2 contain single enum value: SystemAssigned
See https://github.com/Azure/azure-rest-api-specs/blob/master/specification/common-types/resource-management/v2/types.json#L238

I can submit a PR to extend that. What should be the values?

None
SystemAssigned
UserAssigned
SystemAssigned, UserAssigned

Or how Swagger support multiple values?

Choose a reason for hiding this comment

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

please leave as-is for now, we are discussing making the update to the common-types files to support UserAssigned and None

"description": "Identity for the resource.",
"properties": {
"principalId": {
"readOnly": true,
"type": "string",
"description": "The principal ID of resource identity."
},
"tenantId": {
"readOnly": true,
"type": "string",
"description": "The tenant ID of resource."
},
"type": {
"enum": [
"SystemAssigned",
"None"
],
"x-ms-enum": {
"name": "ResourceIdentityType",
"modelAsString": false
},
"type": "string",
"description": "The identity type."
}
}
},
"IotHubSettings": {
"type": "object",
"description": "Device Update account integration with IoT Hub settings.",
Expand Down Expand Up @@ -720,6 +811,10 @@
"AccountUpdate": {
"description": "Request payload used to update and existing Accounts.",
"properties": {
"identity": {
"$ref": "#/definitions/Identity",
"description": "The type of identity used for the resource."
},
"location": {
"type": "string",
"description": "The geo-location where the resource lives"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"accountName": "contoso",
"api-version": "2020-03-01-preview",
"Account": {
"location": "West US 2",
"location": "westus2",
"properties": {}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,23 @@
"tagKey": "tagValue"
}
}
},
"201": {

Choose a reason for hiding this comment

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

1 [](start = 7, length = 1)

202 - no response body

"body": {
"name": "contoso",
"location": "westus2",
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.DeviceUpdate/accounts/contoso",
"type": "Microsoft.DeviceUpdate/accounts",
"identity": {
"principalId": "00000000-0000-0000-0000-000000000000",
"tenantId": "00000000-0000-0000-0000-000000000000",
"type": "SystemAssigned"
},
"properties": {
"provisioningState": "Created",
"hostName": "contoso.api.adu.microsoft.com"
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"parameters": {
"api-version": "2020-03-01-preview",
"subscriptionId": "00000000-0000-0000-0000-000000000000",
"locationName": "westus2",
abatishchev marked this conversation as resolved.
Show resolved Hide resolved
"request": {
"name": "contoso",
"type": "Microsoft.DeviceUpdate/accounts"
}
},
"responses": {
"200": {
"body": {
"nameAvailable": false,
"reason": "AlreadyExists",
"message": "Resource name already exists"
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"parameters": {
"api-version": "2020-03-01-preview",
"subscriptionId": "00000000-0000-0000-0000-000000000000",
"locationName": "westus2",
"request": {
"name": "contoso",
"type": "Microsoft.DeviceUpdate/accounts"
}
},
"responses": {
"200": {
"body": {
"nameAvailable": true,
"reason": "NotSpecified"

Choose a reason for hiding this comment

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

NotSpecified [](start = 19, length = 12)

remove

}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"instanceName": "blue",
"api-version": "2020-03-01-preview",
"Instance": {
"location": "West US 2",
"location": "westus2",
"properties": {
"iotHubs": [
{
Expand All @@ -22,7 +22,7 @@
"201": {
"body": {
"name": "blue",
"location": "West US 2",
"location": "westus2",
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.DeviceUpdate/accounts/contoso/instances/blue",
"type": "Microsoft.DeviceUpdate/accounts/instances",
"properties": {
Expand Down