Arm-Network swagger: added support for IPSec parameters for connection#956
Conversation
|
Hi @henry416, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
|
@henry416 There are references to |
|
@olydis I was under the impression that api changes were partial - I have moved everything applicable over into the new folder. |
|
@olydis Is there any issues remaining with this PR? |
|
@henry416 Will review shortly! |
There was a problem hiding this comment.
make sure that the version property in every swagger matches the api-version in file path.
|
@henry416 - This file also needs to be updated https://github.com/Azure/azure-rest-api-specs/blob/master/arm-network/compositeNetworkClient.json. Btw, why did you blindly move everything to 2017-03-01 api-version? Shouldn't you move only those parts that are on the new api-verison? |
|
@amarzavery My changes are in NRP 2017-03-01. Since this is a new api version, I moved everything applicable into the new directory in order for the check to pass. Am I mistaken in doing so? What is the procedure for new api versions? I really only change the VirtualNetworkGatewayConnection model. |
|
talk to @DeepakRajendranMsft from the Networking team. |
|
@DeepakRajendranMsft any input on this? |
|
I put changes into the current version instead |
|
@amarzavery Can you review the changes while I get the versioning sorted out? The diff will make it easier to see what I have changed. |
There was a problem hiding this comment.
I would like to callout few things:
- These changes are applied to an api-version for which I believe an SDK has already been published. This will call breaking changes in the sdk
- The new properties that you have added are in PascalCase. All the other properties are in camelCase. Please double check what is present on the wire using Fiddler. Looking at the server side code in C# will not help.
@DeepakRajendranMsft - Please take a look.
There was a problem hiding this comment.
@amarzavery I'm actually putting the changes in 2016-12-01 so that it's easier for you to see which parts are being changed through diff. I have discussed with @DeepakRajendranMsft already about how to move to new api-version - I will move all the changes to a new api version once everything is clear.
There was a problem hiding this comment.
The Security Association payload size in KB for a site to site VPN tunnel.
There was a problem hiding this comment.
All the properties are in PascalCase. On the wire everything should be in camelCase as per the convention. JSON object is like a case sensitive dictionary.
"SALifeTimeSeconds" --> "s ALifeTimeSeconds"
There was a problem hiding this comment.
Okay noted. Although a heads up, it seems that there are some schemas that seem to break this convention (see processor architecture under vpnclientparameters).
|
@amarzavery Changes have been made according to your feedback. Let me know if it is good and I will put it in the appropriate api version. |
|
@amarzavery @olydis Please review the changes made according to feedback. |
|
on it |
There was a problem hiding this comment.
Please don't specify the enum values in the description. Some generators will even auto-gen such description and append it (if appropriate), so this is over-specification. Same applies to other enums below.
There was a problem hiding this comment.
@olydis I have changed the descriptions to match. Comment to let me know if that is all and I will move the changes into the new api version.
There was a problem hiding this comment.
what's the deal with these two names?
There was a problem hiding this comment.
The names in NRP are SALifeTimeSeconds
There was a problem hiding this comment.
note that the properties in here have to be called precisely as provided/expected on the wire by your service! if your service sends/expects SALifeTimeSeconds, name it that way :-)
There was a problem hiding this comment.
If that is the case, then the entire network api is named incorrectly. The network service uses upper camel case. But I have done testing with the rest specs and the existing jsons do work with our service.
There was a problem hiding this comment.
@amarzavery this smells like a problem - shouldn't the Swagger match precisely the wire? This will blow up as soon as they provide examples, correct? @henry416 have you tested all generated programming languages/SDKs?
There was a problem hiding this comment.
@olydis I can run the tests for the entire network group, but that will take some time to do.
@DeepakRajendranMsft Do you know why this is the case?
There was a problem hiding this comment.
If that is the case, then the entire network api is named incorrectly. The network service uses upper camel case. But I have done testing with the rest specs and the existing jsons do work with our service.
@henry416 - Please do not take a look at the C# code on the service side. That is what confuses new people all the time. Please open Fiddler and see the actual stuff on the wire and you will find that NRP is sending properties of the request body or the response body in camelCase.
There was a problem hiding this comment.
If that is the case, then the entire network api is named incorrectly.
Just synced with the team. If this is true, this is a HUGE ISSUE. Note that we have tons of things depending on those Swagger definitions, and you are basically saying they are wrong. Swaggers must be accurate all the way down. What have you tested for? Someone may write their own client to your service, look at the Swagger, go JSON.parse(rawBody)["someProp"] but whooops, too bad, your service returned { "SomeProp": 42 }.
There was a problem hiding this comment.
@olydis Ok thanks, I checked the actual responses. False alarm.
@amarzavery Is there any reason why NRP differentiates the actual request properties from the server model?
@henry416 You mean you will handle it as a breaking change? If yes, good. :-) |
BUG! #Resolved Refers to: arm-network/2016-12-01/swagger/virtualNetworkGateway.json:1274 in 67325a1. [](commit_id = 67325a13ff3b2d9211225fbb33c61c15ac467974, deletion_comment = False) |
please add a description #Resolved Refers to: arm-network/2016-12-01/swagger/virtualNetworkGateway.json:1204 in 67325a1. [](commit_id = 67325a13ff3b2d9211225fbb33c61c15ac467974, deletion_comment = False) |
add a description #Resolved Refers to: arm-network/2016-12-01/swagger/virtualNetworkGateway.json:1222 in 67325a1. [](commit_id = 67325a13ff3b2d9211225fbb33c61c15ac467974, deletion_comment = False) |
description missing Refers to: arm-network/2016-12-01/swagger/virtualNetworkGateway.json:1281 in 67325a1. [](commit_id = 67325a13ff3b2d9211225fbb33c61c15ac467974, deletion_comment = False) |
description missing Refers to: arm-network/2016-12-01/swagger/virtualNetworkGateway.json:1588 in 67325a1. [](commit_id = 67325a13ff3b2d9211225fbb33c61c15ac467974, deletion_comment = False) |
|
@olydis Can I create the new api version now? |
|
See #956 (comment) We will not accept inaccurate Swaggers. While services should absolutely adhere to the casing guidelines (i.e. no pascal case), but if they don't, do not create an inaccurate Swagger. |
olydis
left a comment
olydis
left a comment
There was a problem hiding this comment.
Swagger apparently inaccurate
|
@olydis I have tested changes with camelcase and confirmed. Please let me know if that is all that is required. If so, I will move all network swagger to a new API version (2017-03-01) and revert the 2016 version to its original state. |
|
@henry416 Okay, so the Swagger's properties are now spelled precisely as sent/expected by the service? Alright then :-) |
…pi version with ipsec policy changes
|
@olydis @DeepakRajendranMsft I have created the new api version (2017-03-01) and moved all of the IPsec changes into it. Please verify. |
|
Spec used to generate Azure/azure-sdk-for-net#2943 |
|
If @DeepakRajendranMsft is okay with this I will merge. 👍 |
|
@DeepakRajendranMsft Hi Deepak, what is the status on this PR? |
|
@DeepakRajendranMsft Hi, can I get a sign off on this so that I can proceed on the powershell pipeline? |
|
No modification for NodeJS |
|
No modification for Ruby |
|
No modification for NodeJS |
|
No modification for Python |
…iew-update Update/add parameters for WindowsESU MAK key RP
7 participants
parameters
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