-
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
EventHub: 2018-preview - added new API for NetworkRuleSet #4748
Conversation
If you're a MSFT employee, click this link |
Automation for azure-sdk-for-pythonNothing to generate for azure-sdk-for-python |
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
Can one of the admins verify this patch? |
Automation for azure-sdk-for-nodeNothing to generate for azure-sdk-for-node |
Automation for azure-sdk-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-jsNothing to generate for azure-sdk-for-js |
"$ref": "#/parameters/ResourceGroupNameParameter" | ||
}, | ||
{ | ||
"$ref": "#/parameters/NamespaceNameParamet |
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.
For hygiene, resource type name should be lowerCamelCase
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.
resolved
"$ref": "#/parameters/ResourceGroupNameParameter" | ||
}, | ||
{ | ||
"$ref": "#/parameters/NamespaceNameParamet |
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.
Same comment on lowerCamelCasing for hygien
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.
resolved
"$ref": "#/parameters/ResourceGroupNameParameter" | ||
}, | ||
{ | ||
"$ref": "#/parameters/NamespaceNameParamet |
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.
The resource type name should be plural: networkRulesets. I'm not sure what the distinction is between virtualNetworkRules and networkRulsets, but could an improved resource type name help make that distinction?
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.
as this is the preview version, we will be will be deprecated virtualNetworkRules API and will be going ahead with only networkruleset API.
for plural: networkRulesets, Correct me if I am wrong. each namespace will be having only one networkRuleset which will consist of ipRule list and virtualNetworkRule list.
so should we make plural: networkRulesets or singular : networkRuleset ?
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.
yeah, today we do pluralize the resourceType names even though for that type there may be just a single instance possible.
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.
Please see comment about pluralization of resource type name
@KrisBash can you please see the comment for pluralization. changing the definition name of NetworkRuleset to loweCamelCase will change class name to lowerCamelCase which Autorest generates. |
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.
minor typos
} | ||
}, | ||
"200": { | ||
"description": "Namespace successfuly created.", |
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.
successfuly -> successfully
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
...venthub/resource-manager/Microsoft.EventHub/preview/2018-01-01-preview/EventHub-preview.json
Outdated
Show resolved
Hide resolved
} | ||
}, | ||
"200": { | ||
"description": "Namespace successfuly updated.", |
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.
successfuly -> successfully
...venthub/resource-manager/Microsoft.EventHub/preview/2018-01-01-preview/EventHub-preview.json
Outdated
Show resolved
Hide resolved
"$ref": "#/parameters/resourceGroupNameParameter" | ||
}, | ||
{ | ||
"$ref": "#/parameters/namespaceNameParamet |
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.
vaule -> value
"$ref": "#/parameters/resourceGroupNameParameter" | ||
}, | ||
{ | ||
"$ref": "#/parameters/namespaceNameParamet |
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.
NameSpaceVirtualNetworkRuleSetget -> NameSpaceVirtualNetworkRuleSetGet
Or change it to a sentence
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, changed the example filenames from NameSpaceVirtualNetworkRuleSetget to NameSpaceNetworkRuleSetGet
@v-Ajnava - You have updated the model names to be plural, which was not needed. The resource type name in the URL of the API needs to be pluralized. |
@ravbhatnagar updated resource type name in url with plural and lowerCamelCase and reverted plural model names |
cool, signing off from ARM side. |
@KrisBash You need to approve the changes here. I can not merge the changes in without your approval. |
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.
Thanks for the changes
* added new NetworkRule APIs * updated lowerCamelCase for Parameters * pluralize NetworkRuleSets * fixed lint errors * fixed typo and updated examples files names for NetworkRuleSets * updated resource type name in url with plural and lowerCamelCase
) * Swagger for HybridAKS provisioned clusters (#4697) Added swagger for HybridAKS provisioned clusters * Swagger fix validation errors and added examples (#4744) * added examples and fixed some validation errors * fixed more validation errors and warnings * Added x-ms-secret flag for passwords * fixing liniting issues in swagger (#4748) * Fixes for prettier, spell checks and go-sdk generation (#4767) * Fixes for prettier, spell checks and go-sdk generation * changed default network plugin to calico * formatting fix * added enums, defaults and moved nodepool config to a common definition * changed go-sdk to track2 * Added cluster prefix to name and location
Latest improvements:
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.