Skip to content

Common specs fixes #1603

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

Merged
fearthecowboy merged 1 commit into from
Aug 31, 2017
Merged

Common specs fixes #1603

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
4 changes: 2 additions & 2 deletions ...ification/network/resource-manager/Microsoft.Network/2017-08-01/checkDnsAvailability.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"/subscriptions/{subscriptionId}/providers/Microsoft.Network/locations/{location}/CheckDnsNameAvailability": {
"get": {
"operationId": "CheckDnsNameAvailability",
"description": "Checks whether a domain name in the cloudapp.net zone is available for use.",
"description": "Checks whether a domain name in the cloudapp.azure.com zone is available for use.",
"parameters": [
{
"name": "location",
Expand All @@ -51,7 +51,7 @@
{
"name": "domainNameLabel",
"in": "query",
"required": false,
Copy link
Copy Markdown
Contributor

@fearthecowboy fearthecowboy Aug 29, 2017

Choose a reason for hiding this comment

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

Was this a mistake previously?

If they didn't pass it in would this fail?

This shouldn't be a binary breaking change, but it is technically be a source-level-breaking change (since someone could have omitted the parameter and now would have to pass one).

Copy link
Copy Markdown
Contributor

@DeepakRajendranMsft DeepakRajendranMsft Aug 29, 2017

Choose a reason for hiding this comment

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

domainNameLabel is a required property on the service. If users were not passing the parameter, the service would throw an exception.

Yes, it is a breaking change on the client but it is required and i dont expect any customers to get impacted since we were anyway throwing .

This is a bug that we are fixing on the client

Copy link
Copy Markdown
Contributor

@fearthecowboy fearthecowboy Aug 30, 2017

Choose a reason for hiding this comment

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

I figured as much -- it's not likely to cause a breaking change in the generated code unless someone used the function without the parameter, which would have failed... works for me.

"required": true,
"type": "string",
"description": "The domain name to be verified. It must conform to the following regular expression: ^[a-z][a-z0-9-]{1,61}[a-z0-9]$."
},
Expand Down
283 changes: 217 additions & 66 deletions specification/network/resource-manager/Microsoft.Network/2017-08-01/examples/UsageList.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,103 +9,254 @@
"body": {
"value": [
{
"unit": "Count",
"currentValue": 47,
"limit": 100,
"currentValue": 8.0,
Copy link
Copy Markdown
Contributor

@fearthecowboy fearthecowboy Aug 29, 2017

Choose a reason for hiding this comment

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

I noticed these are now decimal values.

in usage.json -- these values are listed as int64 format and you're sending in non-integer values.

I don't think that this is OK -- if the values are coming back as non-integers, some deserializers will probably fail. Is there a service change?

   "Usage": {
      "properties": {
        "unit": {
          "type": "string",
          "description": "An enum describing the unit of measurement.",
          "enum": [
            "Count"
          ],
          "x-ms-enum": {
            "name": "UsageUnit",
            "modelAsString": true
          }
        },
        "currentValue": {
          "type": "integer",
          "format": "int64",
          "description": "The current value of the usage."
        },
        "limit": {
          "type": "integer",
          "format": "int64",
          "description": "The limit of usage."
        },
        "name": {
          "$ref": "#/definitions/UsageName",
          "description": "The name of the type of usage."
        }
      },

Copy link
Copy Markdown
Contributor

@DeepakRajendranMsft DeepakRajendranMsft Aug 29, 2017

Choose a reason for hiding this comment

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

we will not be sending non-integer values. sending non-integer values does not make sense for this api.

To be honest, the service uses this object Microsoft.Azure.Insights.Models.UsageMetric to populate the response and property definitions are as follows
private double _currentValue;
private string _id;
private double _limit;
private LocalizableString _name;
private string _nextResetTime;
private TimeSpan? _quotaPeriod;
private string _unit;

Even though it says double currentValue, it doesnt make sense to introduce a breaking change on our client (from int64 to double).

Copy link
Copy Markdown
Contributor

@fearthecowboy fearthecowboy Aug 30, 2017

Choose a reason for hiding this comment

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

The trouble is, some deserializers may not properly handle deserializing values like 50.0 into an integer field, even if the value is actually a whole number. If your service is returning number values with dots in them, that may not be good for some clients.

I'm more concerned with what the service is returning...

Copy link
Copy Markdown
Contributor

@DeepakRajendranMsft DeepakRajendranMsft Aug 31, 2017

Choose a reason for hiding this comment

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

at this point, i dont know what we can do here. you might have to take it up with the insight people. All usages implementations use that i believe.

Copy link
Copy Markdown
Contributor

@DeepakRajendranMsft DeepakRajendranMsft Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

i thikn the right thing to do would be to convert the clients to double but that would be a breaking change :( . do you think we ought to do that. does this warrant a breaking change for the client? i personally dont think we should introduce a breaking change for this...

Copy link
Copy Markdown
Contributor

@fearthecowboy fearthecowboy Aug 31, 2017

Choose a reason for hiding this comment

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

I think we just stepped into a lose/lose situation.

If the service has been returning floating point numbers all along (even if they are whole numbers) -- that means the clients are broken already.

Which means, we're better off correcting the specification with the correct types and publishing a new version of the package, even if it means that it's a minor breaking change.

Given that the other things in this PR are corrections which may introduce minor breaking effects, my preference is that let's do the right thing and fix the spec and generate the new code.

Copy link
Copy Markdown
Contributor

@DeepakRajendranMsft DeepakRajendranMsft Aug 31, 2017

Choose a reason for hiding this comment

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

i agree with you but the not the timing. With ignite close by, i dont want to do this now. can we do this post ignite? this would require changes in PS as well.

Copy link
Copy Markdown
Contributor

@DeepakRajendranMsft DeepakRajendranMsft Aug 31, 2017

Choose a reason for hiding this comment

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

@fearthecowboy, i have opened an issue for this that we will get to after ignite: #1624

"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/VirtualNetworks",
"limit": 50.0,
"name": {
"value": "VirtualNetworks",
"localizedValue": "Virtual Networks"
}
"localizedValue": "Virtual Networks",
"value": "VirtualNetworks"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 2,
"limit": 20,
"currentValue": 3.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/StaticPublicIPAddresses",
"limit": 20.0,
"name": {
"value": "StaticPublicIPAddresses",
"localizedValue": "Static Public IP Addresses"
}
"localizedValue": "Static Public IP Addresses",
"value": "StaticPublicIPAddresses"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 19,
"limit": 100,
"currentValue": 1.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/NetworkSecurityGroups",
"limit": 100.0,
"name": {
"value": "NetworkSecurityGroups",
"localizedValue": "Network Security Groups"
}
"localizedValue": "Network Security Groups",
"value": "NetworkSecurityGroups"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 43,
"limit": 60,
"currentValue": 8.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/PublicIPAddresses",
"limit": 60.0,
"name": {
"value": "PublicIPAddresses",
"localizedValue": "Public IP Addresses"
}
"localizedValue": "Public IP Addresses",
"value": "PublicIPAddresses"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 33,
"limit": 5000,
"currentValue": 2.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/NetworkInterfaces",
"limit": 350.0,
"name": {
"value": "NetworkInterfaces",
"localizedValue": "Network Interfaces"
}
"localizedValue": "Network Interfaces",
"value": "NetworkInterfaces"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 11,
"limit": 100,
"currentValue": 2.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/LoadBalancers",
"limit": 100.0,
"name": {
"value": "LoadBalancers",
"localizedValue": "Load Balancers"
}
"localizedValue": "Load Balancers",
"value": "LoadBalancers"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 0,
"limit": 50,
"currentValue": 1.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/ApplicationGateways",
"limit": 50.0,
"name": {
"value": "ApplicationGateways",
"localizedValue": "Application Gateways"
}
"localizedValue": "Application Gateways",
"value": "ApplicationGateways"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 3,
"limit": 100,
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/RouteTables",
"limit": 100.0,
"name": {
"value": "RouteTables",
"localizedValue": "Route Tables"
}
"localizedValue": "Route Tables",
"value": "RouteTables"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 0,
"limit": 1000,
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/RouteFilters",
"limit": 1000.0,
"name": {
"value": "RouteFilters",
"localizedValue": "Route Filters"
}
"localizedValue": "Route Filters",
"value": "RouteFilters"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 0,
"limit": 1,
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/NetworkWatchers",
"limit": 1.0,
"name": {
"value": "NetworkWatchers",
"localizedValue": "Network Watchers"
}
"localizedValue": "Network Watchers",
"value": "NetworkWatchers"
},
"unit": "Count"
},
{
"unit": "Count",
"currentValue": 0,
"limit": 10,
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/PacketCaptures",
"limit": 10.0,
"name": {
"value": "PacketCaptures",
"localizedValue": "Packet Captures"
}
"localizedValue": "Packet Captures",
"value": "PacketCaptures"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/DnsServersPerVirtualNetwork",
"limit": 9.0,
"name": {
"localizedValue": "DNS servers per Virtual Network",
"value": "DnsServersPerVirtualNetwork"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/SubnetsPerVirtualNetwork",
"limit": 1000.0,
"name": {
"localizedValue": "Subnets per Virtual Network",
"value": "SubnetsPerVirtualNetwork"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/IPConfigurationsPerVirtualNetwork",
"limit": 4096.0,
"name": {
"localizedValue": "IP Configurations per Virtual Network",
"value": "IPConfigurationsPerVirtualNetwork"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/PeeringsPerVirtualNetwork",
"limit": 10.0,
"name": {
"localizedValue": "Peerings per Virtual Network",
"value": "PeeringsPerVirtualNetwork"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/SecurityRulesPerNetworkSecurityGroup",
"limit": 200.0,
"name": {
"localizedValue": "Security rules per Network Security Group",
"value": "SecurityRulesPerNetworkSecurityGroup"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/SecurityRuleAddressesOrPortsPerNetworkSecurityGroup",
"limit": 2000.0,
"name": {
"localizedValue": "Security rules addresses or ports per Network Security Group",
"value": "SecurityRuleAddressesOrPortsPerNetworkSecurityGroup"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/InboundRulesPerLoadBalancer",
"limit": 150.0,
"name": {
"localizedValue": "Inbound Rules per Load Balancer",
"value": "InboundRulesPerLoadBalancer"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/FrontendIPConfigurationPerLoadBalancer",
"limit": 10.0,
"name": {
"localizedValue": "Frontend IP Configurations per Load Balancer",
"value": "FrontendIPConfigurationPerLoadBalancer"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/OutboundNatRulesPerLoadBalancer",
"limit": 5.0,
"name": {
"localizedValue": "Outbound NAT Rules per Load Balancer",
"value": "OutboundNatRulesPerLoadBalancer"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/RoutesPerRouteTable",
"limit": 100.0,
"name": {
"localizedValue": "Routes per Route Table",
"value": "RoutesPerRouteTable"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/SecondaryIPConfigurationsPerNetworkInterface",
"limit": 256.0,
"name": {
"localizedValue": "Secondary IP Configurations per Network Interface",
"value": "SecondaryIPConfigurationsPerNetworkInterface"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/InboundRulesPerNetworkInterface",
"limit": 500.0,
"name": {
"localizedValue": "Inbound rules per Network Interface",
"value": "InboundRulesPerNetworkInterface"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/RouteFilterRulesPerRouteFilter",
"limit": 1.0,
"name": {
"localizedValue": "Route filter rules per Route Filter",
"value": "RouteFilterRulesPerRouteFilter"
},
"unit": "Count"
},
{
"currentValue": 0.0,
"id": "/subscriptions/subid/providers/Microsoft.Network/locations/westus/usages/RouteFiltersPerExpressRouteBgpPeering",
"limit": 1.0,
"name": {
"localizedValue": "Route filters per Express route BGP Peering",
"value": "RouteFiltersPerExpressRouteBgpPeering"
},
"unit": "Count"
}
]
}
Expand Down
2 changes: 1 addition & 1 deletion specification/network/resource-manager/Microsoft.Network/2017-08-01/networkInterface.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@
"NetworkInterfaces"
],
"operationId": "NetworkInterfaceLoadBalancers_List",
"description": "Get all load balancers in a network interface",
"description": "List all load balancers in a network interface.",
"parameters": [
{
"name": "resourceGroupName",
Expand Down
7 changes: 6 additions & 1 deletion specification/network/resource-manager/Microsoft.Network/2017-08-01/usage.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
"Usages"
],
"operationId": "Usages_List",
"description": "Lists compute usages for a subscription.",
"description": "List network usages for a subscription.",
"parameters": [
{
"name": "location",
Expand Down Expand Up @@ -92,6 +92,11 @@
},
"Usage": {
"properties": {
"id": {
Copy link
Copy Markdown
Contributor

@fearthecowboy fearthecowboy Aug 29, 2017

Choose a reason for hiding this comment

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

This will be a breaking change to a generated class.

Copy link
Copy Markdown
Contributor

@DeepakRajendranMsft DeepakRajendranMsft Aug 29, 2017

Choose a reason for hiding this comment

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

it is a read only string right? why is that considered a breaking change? what do you suggest we do?

this is a bug fix we are trying to do

Copy link
Copy Markdown
Contributor

@fearthecowboy fearthecowboy Aug 30, 2017

Choose a reason for hiding this comment

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

If it's a bugfix, I'm not going to flag it-- it's about as harmless as can be, when introducing a new property into a class where it wasn't before. Likely it's not going to screw anything up.

"type": "string",
"readOnly": true,
"description": "Resource identifier."
},
"unit": {
"type": "string",
"description": "An enum describing the unit of measurement.",
Expand Down