-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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.
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
[Network-2018-02-01] DDoS Protection Plan resource #2567
[Network-2018-02-01] DDoS Protection Plan resource #2567
Conversation
Automation for azure-sdk-for-goEncountered an unknown error: (azure-sdk-for-go)
Traceback (most recent call last):
File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/github_tools.py", line 32, in exception_to_github
yield context
File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 167, in rest_handle_action
return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 182, in rest_pull_close
rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 142, in rest_pr_management
sdk_tag=sdk_tag
File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 283, in generate_sdk_from_git_object
sdk_repo.git.push('origin', base_branch, set_upstream=True)
File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 551, in <lambda>
return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1010, in _call_process
return self.execute(call, **exec_kwargs)
File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 821, in execute
raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(1)
cmdline: git push --set-upstream origin restapi_auto_Network-2018-02-01
stderr: 'To https://AutorestCI:58ab395c311d1bd75b3e1db1cc8adaf9acc42afe@github.com/Azure/azure-sdk-for-go.git
! [rejected] restapi_auto_Network-2018-02-01 -> restapi_auto_Network-2018-02-01 (fetch first)
error: failed to push some refs to 'https://AutorestCI:58ab395c311d1bd75b3e1db1cc8adaf9acc42afe@github.com/Azure/azure-sdk-for-go.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.' |
Automation for azure-sdk-for-pythonA PR has been created for you: |
@ravbhatnagar please review for ARM |
"description": "Indicates if Vm protection is enabled for all the subnets in a Virtual Network." | ||
"enableDdosProtection": { | ||
"type": "boolean", | ||
"description": "Indicates if DDoS protection is enabled for all the protected resources in the virtual network. It requires a DDoS protection plan associated with the resource." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's false by default
"description": "Indicates if DDoS protection is enabled for all the protected resources in the virtual network. It requires a DDoS protection plan associated with the resource." | ||
}, | ||
"enableVmProtection": { | ||
"type": "boolean", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also false by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...it would be a good idea to add the default value to the swagger. This way you will get correct documentation.
"type": "Error", "code": "RequiredPropertiesMissingInResourceModel", "message": "Model definition 'DdosProtectionPlan' must have the properties 'name', 'id' and 'type' in its hierarchy and these properties must be marked as readonly.", "id": "R2020", "validationCategory": "RPCViolation", "providerNamespace": null, "resourceType": null, "sources": [
], "jsonref": "file:///home/travis/build/Azure/azure-rest-api-specs/specification/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json:260:4 ($.definitions.DdosProtectionPlan)", "json-path": "file:///home/travis/build/Azure/azure-rest-api-specs/specification/network/resource-manager/Microsoft.Network/stable/2018-02-01/ddosProtectionPlan.json:260:4 ($.definitions.DdosProtectionPlan)" } |
I see this error that's related to your changes |
"tags": [ | ||
"DdosProtectionPlans" | ||
], | ||
"operationId": "DdosProtectionPlans_List", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename method to DdosProtectionPlans_ListByResourceGroup
? we are trying to have this consistent name for the the generated method that retrieve resources in a subscription.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, see /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/ddosProtectionPlans
"tags": [ | ||
"DdosProtectionPlans" | ||
], | ||
"operationId": "DdosProtectionPlans_ListAll", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename method to DdosProtectionPlans_List
? this is for consistency.
"$ref": "./network.json#/definitions/Resource" | ||
} | ||
], | ||
"description": "A DDoS protection plan in a resource group." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems only properties we can set on DdosProtectionPlan
are location
and name
and tags
. Just trying confirm my understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct
Automation for azure-sdk-for-nodeNothing to generate for azure-sdk-for-node |
Looks good! |
}, | ||
"paths": { | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/ddosProtectionPlans/{ddosProtectionPlanName}": { | ||
"delete": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the response if one or more vnets are referencing ddpp? If they are referencing the dopp, but the enableddpp property is set to false?
@ravbhatnagar, when setting the ddpp property of a vnet to reference a ddpp considered a write operation on the ddpp from a RBAC perspective?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple VNETs can be associated with a DdosProtectionPlan. EnableDdosProtection does not affect the relationship, it only depends on the plan to exist.
You can write only from the VNET side and you need to have permissions to write to the DdosProtectionPlan (you have if it's in the same subscription).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some linter errors reported, but not due to swagger file (ddosProtectionPlan) introduced in this PR but errors from old swagger files such as application gateway, express route.
This reverts commit 9a32d84.
Automation for azure-libraries-for-javaNothing to generate for azure-libraries-for-java |
Note: There are three breaking changes reported by CI, PR adds default value (as false) for enableDdosProtection and enableVmProtection properties in virtualNetwork model and introduces ddosProtectionPlan property in virtualNetwork. diff here. These are not breaking changes in the server - if these properties does not present in the payload then service assumes |
I have no further questions. |
… 2018 (#2642) * Add support for Express Route Circuit Connection resource as a child of Express Route Circuit Peering in 2018-02-01 API (#2358) * copy all files from 2018-01-01 to 2018-02-01 * update api version to 2018 in all files * add express route circuit connection as child of express route bgp peering * update readme.md * add examples * fix travis build error * fix travis build error * fix indentation errors * update reference to circuit connections in peering * fix indentation * Merge Express Route Circuit Connection to Networking2018-02-01 (#2370) * copy all files from 2018-01-01 to 2018-02-01 * update api version to 2018 in all files * add express route circuit connection as child of express route bgp peering * update readme.md * add examples * fix travis build error * fix travis build error * fix indentation errors * update reference to circuit connections in peering * fix indentation * fix indentation * fix indentation' * get latest file * Fix indentation * fix key * fix indentation and example * fix indentation * fix indentation * make circuit con singular * add fix as PR #2364 (#2376) * Network feature: Setting custom ipsec policy for Virtual Network Gateway P2S clients. (#2521) * 1443089: Network feature: Setting custom ipsec policy for Virtual Network Gateway P2S clients. * 1443089:Fix network ReadMe file. * 1443089:Fix network ReadMe file. * Temporary bug fix * Add support for Express Route Provider APIs (#2532) * Add support for Express Route Provider APIs A new top level NRP resource ExpressRouteCrossConnection is being introduced with this change. This resource shadows the customer ExpressRouteCircuit resouce in the customer subscription and enables the provider to perform CRUD operations on some of the sub-resources, such as peerings on the ExpressRoute Circuit * remove wrong enum values from circuit connection status and fix enum … (#2572) * remove wrong enum values from circuit connection status and fix enum name * Update ExpressRouteCrossConnection Route Table Summary Record (#2574) * Update CrossConnection Route Table Summary record * [Network-2018-02-01] DDoS Protection Plan resource (#2567) * Initial version * Add eol * Add extra property in example * Add default values for fields * Rename operation IDs * Make 'id' read-only for all network resources * Revert "Make 'id' read-only for all network resources" * Fix reference * making peer express route circuit peering and express route circuit peering as references (#2696) * Express Route Cross Connection Swagger changes as per PM requirements (#2644) * Fixed operationIds for non-CRUD operations in ExpressRouteCrossConnections (#2720) * Making travis run > 10 mins (#2739)
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-version
in the path should match theapi-version
in the spec).Quality of Swagger