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.

Sign up for GitHub

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

Jump to bottom

[ACR] Add network rule set properties, empty resource group validation #4506

Merged
merged 1 commit into from
Nov 30, 2018
Merged

[ACR] Add network rule set properties, empty resource group validation #4506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ...rce-manager/Microsoft.ContainerRegistry/preview/2017-06-01-preview/containerregistry.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1856,7 +1856,7 @@
"type": "integer"
},
"digest": {
"description": "The digest of the content, as defined by the Registry V2 HTTP API Specificiation.",
"description": "The digest of the content, as defined by the Registry V2 HTTP API Specification.",
"type": "string"
},
"length": {
Expand Down
1 change: 1 addition & 0 deletions ...try/resource-manager/Microsoft.ContainerRegistry/stable/2017-03-01/containerregistry.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,7 @@
"description": "The name of the resource group to which the container registry belongs.",
"required": true,
"type": "string",
"minLength": 1,
"x-ms-parameter-location": "method"
},
"RegistryNameParameter": {
Expand Down
51 changes: 51 additions & 0 deletions ...try/resource-manager/Microsoft.ContainerRegistry/stable/2017-10-01/containerregistry.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,10 @@
"storageAccount": {
"$ref": "#/definitions/StorageAccountProperties",
"description": "The properties of the storage account for the container registry. Only applicable to Classic SKU."
},
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the service behavior if a client performs a PUT with this API version and specifies networkRuleset, and another client GETs & PUTs the resource with a lower API version? Is the downlevel PUT blocked, or do you ignore networkRuleset for the update?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lower versions ignore networkRuleset in PUT/PATCH, also won't return it in GET.

Copy link
Member Author

Choose a reason for hiding this comment

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

@KrisBash Could you please advise if this is the expected behavior? We have an approaching release date and would like to have this PR merged ASAP. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems reasonable to me. @KrisBash - signing off on your behalf.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, did not check thouroughly. @djyou - seems like this new property is added in the same api-version. If your service were doing the right PUT behavior (replace semantics) then this would be a breaking change. I would recommend doing this in a new API-version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is exactly what I meant. Currently your RP does not implement PUT in the way it should have been where properties missing in the request are wiped out from the resource on the server. So in essence, following the "replace" semantics for PUT. Since your RP is not following the right model, it is currently not a breaking change. But for most services (specially the newer ones) in Azure, such changes go in a new api-version and PUT is a replace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @ravbhatnagar We had an offline discussion within the team, since historically this resource didn't follow the replace semantic and introducing the right pattern itself is a breaking change (due to the existing properties adminUserEnabled, storageAccountProperties, etc.), do you think we can have this merged in for this api-verison, and we will introduce a new api-version that follows the right pattern? /cc @mnltejaswini @sajayantony

Copy link
Contributor

Choose a reason for hiding this comment

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

@ravbhatnagar We queued up an item to redesign the resource APIS with a newer api version and other new features. As part of it, we can update the APIs to implement the semantics correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Signing off from ARM side. opened an issue to track it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

"networkRuleSet": {
"$ref": "#/definitions/NetworkRuleSet",
"description": "The network rule set for a container registry."
}
}
},
Expand Down Expand Up @@ -1535,6 +1539,48 @@
}
}
},
"NetworkRuleSet": {
KrisBash marked this conversation as resolved.
Show resolved Hide resolved
"description": "The network rule set for a container registry.",
"required": [
"defaultAction"
],
"type": "object",
"properties": {
"defaultAction": {
"description": "The default action of allow or deny when no other rules match.",
"default": "Allow",
"enum": [
"Allow",
"Deny"
],
"type": "string",
"x-ms-enum": {
"name": "DefaultAction",
"modelAsString": true
}
},
"virtualNetworkRules": {
"description": "The virtual network rules.",
"type": "array",
"items": {
"$ref": "#/definitions/VirtualNetworkRule"
}
}
}
},
"VirtualNetworkRule": {
"description": "The virtual network rule for a container registry.",
"required": [
"id"
],
"type": "object",
"properties": {
"id": {
"description": "Resource ID of a subnet, for example: /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName}/subnets/{subnetName}.",
"type": "string"
}
}
},
"RegistryUpdateParameters": {
"description": "The parameters for updating a container registry.",
"type": "object",
Expand Down Expand Up @@ -1568,6 +1614,10 @@
"storageAccount": {
"$ref": "#/definitions/StorageAccountProperties",
"description": "The parameters of a storage account for the container registry. Only applicable to Classic SKU. If specified, the storage account must be in the same physical location as the container registry."
},
"networkRuleSet": {
"$ref": "#/definitions/NetworkRuleSet",
"description": "The network rule set for a container registry."
}
}
},
Expand Down Expand Up @@ -2370,6 +2420,7 @@
"description": "The name of the resource group to which the container registry belongs.",
"required": true,
"type": "string",
"minLength": 1,
"x-ms-parameter-location": "method"
},
"RegistryNameParameter": {
Expand Down
3 changes: 2 additions & 1 deletion ...source-manager/Microsoft.ContainerRegistry/stable/2018-09-01/containerregistry_build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,7 @@
}
},
"Task": {
"description": "The task that has the ARM resource and task properties. \r\nThe task will have all information to schedule a run against it.",
"description": "The task that has the ARM resource and task properties. \r\nThe task will have all information to schedule a run against it.",
"type": "object",
"allOf": [
{
Expand Down Expand Up @@ -2194,6 +2194,7 @@
"description": "The name of the resource group to which the container registry belongs.",
"required": true,
"type": "string",
"minLength": 1,
"x-ms-parameter-location": "method"
},
"RegistryNameParameter": {
Expand Down