Added swagger for Azure container registry #549
Conversation
|
Hi @djyou, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
Please make sure that the above two steps are done before we start the review. |
|
/cc @sajayantony @sivagms |
| "version": "2016-06-27-preview", | ||
| "title": "ContainerRegistry" | ||
| }, | ||
| "host": "https://management.azure.com", |
| }, | ||
| "host": "https://management.azure.com", | ||
| "schemes": [ | ||
| "http" |
| "consumes": [], | ||
| "produces": [ | ||
| "application/json", | ||
| "text/json" |
There was a problem hiding this comment.
does the service actually support text/json? We had tried this with another Azure RP and it returned content-type "text/json" not supported. Hence wanted to ensure that this is really supported by the service.
| "name": "resourceGroup", | ||
| "in": "path", | ||
| "required": true, | ||
| "type": "string" |
There was a problem hiding this comment.
please make sure you provide description for all the parameters, operations, response model definitions, model properties in the definition. The description translates into documentation for the generated client library. This will also turn into REST API docs for your API.
| "200": { | ||
| "description": "OK", | ||
| "schema": { | ||
| "$ref": "#/definitions/Object" |
There was a problem hiding this comment.
Why would you return an "Object"? Please stop using swashbuckle and hand author your swagger. Swashbuckle generates bad swaggers that are difficult to consume.
| } | ||
| } | ||
| }, | ||
| "Object": { |
There was a problem hiding this comment.
Why do you need this model?
| "ResourceList[RegistryParameters]": { | ||
| "type": "object", | ||
| "properties": { | ||
| "value": { |
There was a problem hiding this comment.
value should be a required property.
| "SubscriptionIdParameter": { | ||
| "name": "subscriptionId", | ||
| "in": "path", | ||
| "description": "Gets subscription credentials which uniquely identify Microsoft Azure subscription.The subscription ID forms part of the URI for every service call.", |
There was a problem hiding this comment.
Keep the description short and simple
"The Azure subscription id."
| "tags": [ | ||
| "Registries" | ||
| ], | ||
| "operationId": "Registries_List", |
There was a problem hiding this comment.
please add the extension "x-ms-pageable". You can find more information about this doscument over here. Everything is documented, so please make sure to read the documentation correctly and then model the extension as per your use case.
| "in": "body", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/RegistryParameters" |
There was a problem hiding this comment.
make sure that the body parameter of the patch operation has all optional properties, no readonly properties and no defaults.
| @@ -0,0 +1,361 @@ | |||
| { | |||
There was a problem hiding this comment.
General Comment: The operations, parameters, models, modelProperties should all be well documented by providing accurate description.
|
@djyou - What is the status of this PR ? When can the review comments be addressed? |
|
Our RP is making several major changes and we are working on the swagger file. We will try to finish this ASAP. |
|
@djyou - Any further update? |
|
@amarzavery I believe we have addressed requested changes as much as we can. We appreciate any additional feedback. Thanks! /cc @sajayantony |
|
/cc @JasonRShaver |
| "schemes": [ | ||
| "https" | ||
| ], | ||
| "paths": { |
There was a problem hiding this comment.
you can have a top level produces and consumes.
"consumes": [
"application/json"
],
"produces": [
"application/json"
],
Then you don't need to define it explicitly for every operation. The operation can surely override the global produces and consumes if required
| "Operation" | ||
| ], | ||
| "description": "Checks whether the container registry name is available.", | ||
| "operationId": "Operation_CheckNameAvailability", |
There was a problem hiding this comment.
Registries_CheckNameAvailability
There was a problem hiding this comment.
Our RP explicitly put this under Operation since the path goes to Microsoft.ContainerRegistry/checkNameAvailability rather than, for example, Microsoft.ContainerRegistry/registries/{registryName}. I have seen different patterns. For example, storage account put checkNameAvailability under StorageAccount, while CDN put it under a special NameAvailability: https://github.com/Azure/azure-rest-api-specs/blob/master/arm-cdn/2016-10-02/swagger/cdn.json#L1594 Is this a required change or can we keep it as operation?
| "type": "string" | ||
| }, | ||
| "type": { | ||
| "description": "The container registry resource type.", |
There was a problem hiding this comment.
What are the possible values ? This looks like an enum candidate. How do you expect the customer to know what should be provided.
Tip: Please wear the hat of a customer while authoring the spec. That will push you to author a better swagger spec.
| "tags": [ | ||
| "Registries" | ||
| ], | ||
| "description": "Returns the properties for the specified container registry.", |
There was a problem hiding this comment.
Returns Gets the properties for the specified container registry.
| "Registries" | ||
| ], | ||
| "description": "Creates a new container registry with the specified parameters.", | ||
| "operationId": "Registries_Create", |
There was a problem hiding this comment.
What will happen if the customer uses the PUT operation to do an UPDATE (He/she makes a 2nd put call)? Will the service throw an error or will the service replace the ContainerRegistry with the new payload provided in the request.
If the service allows the update then should this actually be CreateOrUpdate ?
There was a problem hiding this comment.
Right, our RP will try to do an update if the resource already exists. The issue of making it CreateOrUpdate is that, create and update use different parameters (although technically update uses a subset of parameters from create). In our case, for example, location is a required property in create, but is forbidden to change in update. If the user happens to PUT the correct set of parameters for an existing resource, the RP will return success; otherwise, the RP still returns failure. What is the recommended pattern here?
| } | ||
| } | ||
| }, | ||
| "StorageAccountBase": { |
There was a problem hiding this comment.
Why is this model named as StorageAccountBase? Why cant this be named as StorageAccount ?
There was a problem hiding this comment.
We used StorageAccount for a different purpose in our RP and here StorageAccountBase, as a response to the user, will only contain storage account name.
| "location": { | ||
| "description": "Resource location.", | ||
| "type": "string", | ||
| "readOnly": true |
There was a problem hiding this comment.
location cannot be readOnly
There was a problem hiding this comment.
location must be required.
There was a problem hiding this comment.
I read the documentation suggesting location to be required in Resource before making the changes and I was actually puzzled. When we create/update a registry, we use RegistryCreateParameters/RegistryUpdateParameters. The Registry definition (which inherits Resource) will only be used in response. In that sense, why do we want to make it required? We were following the same pattern from storage account and they didn't have location as a required property (https://github.com/Azure/azure-rest-api-specs/blob/master/arm-storage/2016-01-01/swagger/storage.json#L948). Futher, shouldn't location be treated in the same way as, for example, resource name, since once they are set, they cannot be changed, unless the resource is recreated? Shouldn't location be readOnly then?
There was a problem hiding this comment.
To give you an idea, this is the model I was envisioning for StorageManagement spec and I believe Container Registry will have a very similar model.
- CreateOrUpdate {PUT} (body parameter StorageAccount) -> StorageAccount
- Update {PATCH} (body parameter MutableStorageAccount) -> StorageAccount
- Get {GET} () -> StorageAccount
Here, visualize
StorageAccountasContainerRegistryStorageAccountPropertiesasContainerRegistryProperties
MutableStorageAccountasContainerRegistryUpdateParametersMutableStorageAccountPropertiesasContainerRegistryPropertiesUpdateParameters
- CreateOrUpdate {PUT} (body parameter ConatinerRegistry) -> ConatinerRegistry
- Update {PATCH} (body parameter ConatinerRegistryUpdateParameters) -> ConatinerRegistry
- Get {GET} () -> ConatinerRegistry
What do you think about it?
There was a problem hiding this comment.
@amarzavery Thanks a lot for the detailed suggestion, this is great and we will definitely adopt this model and make improvements. I just want to make sure my understanding of the scenario is correct using the same model as input/output for PUT.
GET | PUT (or even PUT | PUT), if the user modified location (since location is required and not readOnly), the RP will throw an error. We don't seem to have a way to inform the user that location cannot be changed for an existing resource, or is this considered a common sense to Azure resources?
The pipeline scenario you mentioned earlier GET | UPDATE, the user can only do this by calling a PUT after GET, if the above scenario is enabled. We can't have GET | PATCH, since PATCH uses a different model as input and all properties are optional.
| "StorageAccountProperties": { | ||
| "description": "The storage account properties.", | ||
| "required": [ | ||
| "name", |
There was a problem hiding this comment.
Why is the name required? Shouldn't the accessKey and the endpointUrl be enough to uniquely identify and access that storageAccount ?
There was a problem hiding this comment.
Right, accessKey and endpointUrl are enough to identify and access one storage account. Our RP needs to query storage account properties from name or resource id. We also want to support changing storage account for a registry, and in the future, map multiple storage accounts to one registry. It is just easier to manage with storage account name as required property.
| "description": "Storage account endpoint URL.", | ||
| "type": "string" | ||
| }, | ||
| "location": { |
There was a problem hiding this comment.
Why does the service even ask for location? It is optional. What good does it do to the service when it is provided to the service?
There was a problem hiding this comment.
One of our requirements is that the registry has to be in the same location as the storage account. So either the request will have location or the RP will query storage account location using storage account name (also the reason why we want to have storage account name as a property for registry). We probably want to remove it in the future, since there is no guarantee the provided location is the actual location of the storage account, so we made it optional here.
| "type": "string" | ||
| }, | ||
| "adminUserEnabled": { | ||
| "description": "The boolean value that indicates whether admin user is enabled.", |
There was a problem hiding this comment.
Since this is optional. Please specify the default behavior in the description.
"The boolean value that indicates whether admin user is enabled. Default value is false."
| "storageAccount" | ||
| ], | ||
| "properties": { | ||
| "tenantId": { |
There was a problem hiding this comment.
@djyou - tenantId is not sensitive information like storage account access keys; then why is tenantId not returned in the response, like adminUserEnabled ?
The reasoning behind asking this question is -
We want to make it easy for the customer to update the registry. If we have the same model than it becomes easy to enable the pipeline scenario: GET | UPDATE; where the customer can do a GET, modify some properties of the object and then do a PUT. If tenantId is also returned in the response then it is very simple to operate.
There was a problem hiding this comment.
This is a reasonable use scenario, @mnltejaswini, can also we return tenantId? Actually, the response from GET is Registry, the parameters for PATCH and PUT are RegistryUpdateParameters and RegistryCreateParameters. We won't return storage account access key and endpoint url, but we also allow the users to update them. Besides, I doubt if this kind of pipeline will ever work in our case. We were following the same pattern from StorageAccount, for example, here "sku" is readOnly, but here "sku" can also be updated. We also set all properties in Registry to be readOnly, that means the user cannot change them and send the updated registry back. The only way to do this is to use RegistryUpdateParameters. This also corresponds to the discussion on CreateOrUpdate, I think.
There was a problem hiding this comment.
We are planning to revamp the storage management spec. It has not been modeled accurately. So having that as a reference check is not ideal.
Yes, I was thinking about StorageAccount. As per the current model accessKey, endPointUrl and location are write-only (that is they can be created and updated) but cannot be read. Currently swagger does not provide a mechanism to specify write-only.
Actually, only accessKey contains sensitive information. What is the harm in sending the endPointUrl and location back in the response?
Ideally StorageAccountBase should have { name, location, endPointUrl } as required properties.
StorageAccountProperties { accessKey (required) } should inherit from StorageAccountBase.
RegistryProperties.storageAccount should be of type StorageAccountProperties. So when the service sends the response it will not have accessKeys, which I think should be fine as we do not validate on deserialization.
So having Registry as input to PUT and output of PUT, you will make customers life easy and open up the CreateOrUpdate() scenario on PUT.
The input model for PATCH as an update is good.
|
@amarzavery Thank you for your great feedback in the email and we have made suggested changes. However, as @sivagms suggested in the email, we will use the same Registry model as the input and output for PUT, so that our Create will actually be CreateOrUpdate, but we will return null for storage account accessKey in the response. Since VM also has the similar model for admin password, we consider it as acceptable. Besides, once we move to first party, we will remove accessKey and endpointUrl from storage account properties. |
|
@JasonRShaver We want to get this spec reviewed and ironed out so that we can iterate on the RP this week. |
| "tags": [ | ||
| "Registries" | ||
| ], | ||
| "description": "Creates a new container registry with the specified parameters.", |
There was a problem hiding this comment.
Creates or updates a new container registry with the specified parameters. Please make sure that the storage account that is provided in this operation is created in the same location in which the Registry will be created.
|
LGTM |
amarzavery
changed the title
1. Added regenerate credentials operation. 2. Updated storage account properties.
| @@ -323,6 +323,46 @@ | |||
| }, | |||
| "deprecated": false | |||
| } | |||
| }, | |||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroup}/providers/Microsoft.ContainerRegistry/registries/{registryName}/regenerateCredentials": { | |||
There was a problem hiding this comment.
instead of resourceGroup the name of the path parameter should be resourceGroupName. This will be consistent with other specs.
|
@amarzavery We need to freeze on these changes this week. Do you think that would be reasonable. |
| } | ||
| } | ||
| }, | ||
| "deprecated": false |
There was a problem hiding this comment.
"deprecated": false [](start = 8, length = 19)
Why do you have this everywhere? False is default value anyway.
| "deprecated": false, | ||
| "x-ms-pageable": { | ||
| "nextLinkName": null | ||
| } |
There was a problem hiding this comment.
Is there any specific reason to disable Pagination? Keep in mind that if at some point in the future server will return more than 1 page of results you will have to make a breaking change in all the SDK's.
There was a problem hiding this comment.
nextLink is not a required field and can be null in the server response. So no need to do any work on the server side. Specifying a filed name for nextLinkName will require you to modify the return value (adding a filed with that name) and will change method return type to IPage.
There was a problem hiding this comment.
@hovsepm - That would be inconsistent with our primary goal "Swagger spec should accurately describe what is there on the wire". Going from non-pageable to pageable is a breaking change. Wouldn't it be correct to consume a breaking change in the API because of that (whenever that happens) ?
There was a problem hiding this comment.
@amarzavery Supporting pages should not be a breaking change on the SDK side IMO, it is a server side enhancement. The next link field is optional always, so the model will still describe accurately what is on the wire :).
| }, | ||
| "deprecated": false, | ||
| "x-ms-pageable": { | ||
| "nextLinkName": null |
There was a problem hiding this comment.
"nextLinkName": null [](start = 10, length = 20)
Same comment. Are you going to lock number of container registries at "no more than one serve page" in the subscription?
There was a problem hiding this comment.
@hovsepm can the swagger have "nextLinkName": "nextLink" does the RP need to return the field since it doesn't support pagenation at this point.
There was a problem hiding this comment.
The RP must send the "nextLink" property in the response only when there is more information to be provided in the nextPage. Otherwise the structure would be {"value": [ . . . ] }.
| "x-ms-pageable": { | ||
| "nextLinkName": null | ||
| "nextLinkName": "nextLink" |
There was a problem hiding this comment.
Just adding this thing will not be enough. You need to modify RegistryListResult type as well. It should look like this:
"RegistryListResult": {
"description": "The result of listing container registries.",
"properties": {
"value": {
"type": "array",
"items": {
"$ref": "#/definitions/Registry"
}
},
"nextLink": {
"type": "string",
"description": "Gets or sets the URL to get the next set of results."
}
}
}
|
@djyou please open you swagger in VS code with Swagger linter extension installed and fix all "missing description" warnings. Please ignore this comment if you have already done that. |
|
@hovsepm We had used swagger linter to fix issues, is there anything we didn't catch in the current swagger? |
|
@djyou No, just noticed that RegistryListResult's"value" field does not have any description and thought that you may have forgotten to run linter. Then please ignore my previous comment. |
|
LGTM |
|
|
| } | ||
| ], | ||
| "properties": { | ||
| "properties": { |
There was a problem hiding this comment.
one more thing.
"properties" property of "Registry" must also be required. "RegistryProperties" is flattened. It has a required property "storageAccount". This must be the required property of "Registry" after flattening. Hence, it is very important to make "properties" of "Registry" required.
There was a problem hiding this comment.
RegistryProperties is flattened and it will not show up in the generated SDK. How does this matter? For example, the Python SDK generated will have
def __init__(self, location, storage_account, tags=None, admin_user_enabled=False):
which is correct as location and storage_account are the required properties.
There was a problem hiding this comment.
Ahh I see.. Since there is only one property it is working fine. I think if more than one property is required in the object that is supposed to be flattened then it will be a problem. Hence it is safer to mark properties required as well.
There was a problem hiding this comment.
I have an impression that required is redundant to x-ms-client-flatten because the object will go away anyway.
There was a problem hiding this comment.
@amarzavery can you or your team help merge this PR . We can open issues and track anything needed on top of this. Please assign to me or and @djyou for anything you need fixed.
djyou
changed the title
|
@vishrutshah -Do not publish a ruby gem for container registry right now after the rest-api-specs PR is merged. |
|
No modification for NodeJS |
|
No modification for Ruby |
|
No modification for Python |
|
@amarzavery Thanks for the heads-up. :) Just curious what are the reasons? |
|
@vishrutshah - The service is not fully ready yet. It will be tested in the new CLI and then we will get a green signal. |
This change is