Initial network error fixes and suppressions #3051
Conversation
EvgenyAgafonchikov
requested review from
DeepakRajendranMsft and
MikhailTryakhov
as code owners
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
| ## Suppression | ||
| ``` yaml | ||
| directive: | ||
| - suppress: RequiredPropertiesMissingInResourceModel |
There was a problem hiding this comment.
@EvgenyAgafonchikov were these suppressions approved? Please forward the email thread so we can verify
MikhailTryakhov
changed the title
| ## Suppression | ||
| ``` yaml | ||
| directive: | ||
| - suppress: RequiredPropertiesMissingInResourceModel |
There was a problem hiding this comment.
Can we fix it for APIs going forward? So can we suppress it for existing API-versions. And then going forward, new API-versions reference the "resource" model where "id" property is marked as read-only. Would it cause any issues on SDK generation side? @dsgouda ?
There was a problem hiding this comment.
Please add these suppressions to the specific APis or api-versions and not for the entire thing.
There was a problem hiding this comment.
@ravbhatnagar reworked. Please review
There was a problem hiding this comment.
Hmm... theoretically, yes. Any property not marked as readonly could be "set" by a user facing sdk. In this case it would include id as well. If existing APIs have some way of detecting an incorrectly set "id" and throw an appropriate error we should be OK. If not, I am not sure what the implications could be.
@fearthecowboy any thoughts here?
There was a problem hiding this comment.
The constructors for model types would likely change ... so it'd be a binary breaking change.
other than that, the worst that would happen is that someone would change a value, and the server give them an error for trying.
There was a problem hiding this comment.
So lack of readonly property won't raise serious problems. No breaking changes would be done. Everyone happy?!!
There was a problem hiding this comment.
IIRC
- We want to enable these suppressions only for the older API versions, maybe we can enable suppressions only for api tags prior to a certain tag, please consult on the offline thread for this since I am not the expert here
- All new API versions MUST adhere to these rules
- If we merely decide to enable suppressions and not change the spec/api for older versions then yes, there will be no breaking changes introduced, however, newer api versions will definitely cause breaking changes to the subsequently generated sdks.
Hope this makes sense
There was a problem hiding this comment.
Yes, put the directive section with the suppress items in the tag'd sections for that specific API version. Then it will only apply on that version.
There was a problem hiding this comment.
@dsgouda why do we need these suppressions for older api only? As I understand there is no chance we'll make ids readonly so we'll need to have these exceptions in all the versions... am I missing something?
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
| @@ -410,6 +410,96 @@ input-file: | |||
| - Microsoft.Network/preview/2015-05-01-preview/network.json | |||
| ``` | |||
|
|
|||
| ## Suppression | |||
| ``` yaml | |||
There was a problem hiding this comment.
@MikhailTryakhov For conditional suppressions, you simply add conditions like so
yaml $(tag) == 'package-2015-06 || $(tag) == 'package-2015-05-preview`
and so on until all the tags are included in the condition.
@fearthecowboy does that sound right?
There was a problem hiding this comment.
@dsgouda is this to restrict version where we apply this exception?
as I understood @ravbhatnagar approved this to apply on all the versions cause we must change it in older version and don't plan to change in future (huge breaking change)
|
@EvgenyAgafonchikov please update the branch and we should be good to merge |
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
Automation for azure-libraries-for-javaA PR has been created for you: |
Automation for azure-sdk-for-nodeA PR has been created for you: |
Automation for azure-sdk-for-pythonEncountered a Subprocess error: (azure-sdk-for-python)
Command: ['/usr/local/bin/autorest', '/tmp/tmpvc2h6ik7/rest/specification/network/resource-manager/readme.md', '--multiapi', '--python', '--python-mode=update', '--python-sdks-folder=/tmp/tmpvc2h6ik7/sdk', '--use=@microsoft.azure/autorest.python@~3.0', '--version=preview'] AutoRest code generation utility [version: 2.0.4262; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Loading AutoRest core '/root/.autorest/@microsoft.azure_autorest-core@2.0.4278/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4278)
Loading AutoRest extension '@microsoft.azure/autorest.python' (~3.0->3.0.51)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
Processing batch task - {"tag":"package-2018-02"} .
Processing batch task - {"tag":"package-2018-01"} .
Shutting Down
Process() cancelled due to exception : Connection is closed.
Failure during batch task - {"tag":"package-2018-01"} -- Error: Connection is closed..
Error: Connection is closed. |
MikhailTryakhov
force-pushed
the
add-suppression
branch
from
95c2c27
to
1657836
Compare
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/automation/resource-manager/readme.md
|
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/automation/resource-manager/readme.md
|
| Code | Id | Source | Message |
|---|---|---|---|
| PageableOperation | R2029 | Link | Based on the response model schema, operation 'BlobContainers_List' might be pageable. Consider adding the x-ms-pageable extension. |
| PutRequestResponseScheme | R2017 | Link | A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'StorageAccounts_Create' Request Model: 'StorageAccountCreateParameters' Response Model: 'StorageAccount' |
| OperationIdNounConflictingModelNames | R2063 | Link | OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'UsageModel'. Consider using the plural form of 'Usage' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. |
| DescriptionAndTitleMissing | R4000 | Link | 'Resource' model/property lacks 'description' and 'title' property. Consider adding a 'description'/'title' element. Accurate description/title is essential for maintaining reference documentation. |
❌0 new Errors.(0 total)
File: specification/web/resource-manager/readme.md
⚠️ 0 new Warnings.(640 total)
❌0 new Errors.(0 total)
AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback
Thanks for your co-operation.
Automation for azure-sdk-for-goNothing to generate for azure-sdk-for-go |
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
MikhailTryakhov
added
Network
DoNotMerge
MikhailTryakhov
force-pushed
the
add-suppression
branch
from
8219f63
to
ee14327
Compare
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
MikhailTryakhov
removed
the
DoNotMerge
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
1 similar comment
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/network/resource-manager/readme.md
|
|
@dsgouda could you please merge? |
* Added 2018-05-01 version. Common fixes (#3025) * Created May folder. Updated version for exampls, commands * Updated package to May version * Returned NW examples * Fixed API version; Fixed NetworkWatcher examples * Fixed VPN Gateway/Connection examples * Add changes from PR#2975 https://github.com/Azure/azure-rest-api-specs/pull/2975/files * Adding optional traffic analytics config fields to network watcher config object for enhancing the Network Watcher cmdlets (#3023) * Adding optional traffic analytics parameters with existing network watched flowlog configuration * Updating the version * Updating latest versoin in readme.md * #AppGw Fundamentals Clean Up (#3046) * Network 2018 05 01 (#3068) * Adding optional traffic analytics parameters with existing network watched flowlog configuration * Updating the version * Updating latest versoin in readme.md * Adding optional traffic analytics parameters with flowlog parameters * Reverting old chnages * Expose new SKUs for VMSS VPN and ER gateways (#3069) Expose new SKUs for VMSS VPN and ER gateways * Initial network error fixes and suppressions (#3051) * Initial network error fixes and suppressions * reworked exception to specified code blocks only * Added express route fixes
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