-
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
Dsc paging changes #2931
Dsc paging changes #2931
Conversation
Automation for azure-libraries-for-javaNothing to generate for azure-libraries-for-java |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
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
|
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
|
Automation for azure-sdk-for-goEncountered a Subprocess error: (azure-sdk-for-go)
Command: ['/usr/local/bin/autorest', '/tmp/tmp756ke8o6/rest/specification/automation/resource-manager/readme.md', '--go', '--go-sdk-folder=/tmp/tmp756ke8o6/src/github.com/Azure/azure-sdk-for-go', '--multiapi', '--use=@microsoft.azure/autorest.go@~2.1.98', '--use-onever', '--verbose'] 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.4272/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4272)
Loading AutoRest extension '@microsoft.azure/autorest.go' (~2.1.98->2.1.98)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
Processing batch task - {"tag":"package-2015-10"} .
Processing batch task - {"tag":"package-2017-05-preview"} .
Processing batch task - {"tag":"package-2018-01-preview"} .
FATAL: System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: startIndex
at System.String.LastIndexOf(Char value, Int32 startIndex, Int32 count)
at AutoRest.Go.Model.CodeModelGo.PackageVerDir() in /opt/vsts/work/1/s/src/Model/CodeModelGo.cs:line 64
at AutoRest.Go.Model.CodeModelGo.get_DefaultUserAgent() in /opt/vsts/work/1/s/src/Model/CodeModelGo.cs:line 83
at AutoRest.Go.Model.CodeModelGo.get_UserAgent() in /opt/vsts/work/1/s/src/Model/CodeModelGo.cs:line 31
at AutoRest.Go.Templates.VersionTemplate.<ExecuteAsync>d__1.MoveNext() in /opt/vsts/work/1/s/src/obj/Razor/Templates/VersionTemplate.cshtml:line 28
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at AutoRest.Core.CodeGenerator.<Write>d__12.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at AutoRest.Go.CodeGeneratorGo.<Generate>d__6.MoveNext() in /opt/vsts/work/1/s/src/CodeGeneratorGo.cs:line 91
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at AutoRest.Go.Program.<ProcessInternal>d__3.MoveNext() in /opt/vsts/work/1/s/src/Program.cs:line 107
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at NewPlugin.<Process>d__15.MoveNext()
FATAL: go/generate - FAILED
FATAL: Error: Plugin go reported failure.
Process() cancelled due to exception : Plugin go reported failure.
Failure during batch task - {"tag":"package-2018-01-preview"} -- Error: Plugin go reported failure..
Error: Plugin go reported failure. |
}, | ||
"totalCount": { | ||
"type": "integer", | ||
"description": "Gets the total number of records matching filter criteria." |
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.
filter [](start = 68, length = 6)
Which filter?
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.
If get call includes a $filter then gets the total number of records matching that filter. If no $filter specification, then gets the total number of records.
The $filter parameter specifies "The filter to apply on the operation." The paging examples show various values for $filter and what happens when no $filter is specified
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.
@mcardosos -- amended description to read "Gets the total number of records matching countType criteria." where countType is a defined query parameter
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 $filter
parameter is missing in the operation definition
Also, there are some model validation errors |
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.
Added some comments
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
|
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
|
@mcardosos -- changed description on NodeCounts from "Gets the total number of records matching filter criteria." to "Gets the total number of records matching countType criteria." |
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
|
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
|
@mcardosos -- fixed description (your review comment) to read "Gets the total number of records matching countType criteria." where countType is defined as a query parameter with description: "The type of counts to retrieve" |
@@ -366,6 +372,34 @@ | |||
}, | |||
{ | |||
"$ref": "./definitions.json#/parameters/ApiVersionParameter" | |||
}, | |||
{ | |||
"name": "$filter", |
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.
$filter [](start = 21, length = 7)
What about adding the x-ms-odata
extension? #Resolved
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.
@mcardosos -- added x-ms-odata extension per request |
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
|
} | ||
}, | ||
"paths": { | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Automation/automationAccounts/{automationAccountName}/nodecounts/{countType}": { |
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.
why is nodeCounts modeled as a separate resource type? Could it be just a property on the automation Account or on nodes?
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.
@ravbhatnagar - Not sure what you mean by "a property on the AutomationAccount or on nodes" but the following was my reasoning for a separate resource...
/subscriptions/{SubId}/.../nodes specifies getting a list of all nodes and /subscriptions/{SubId}/.../nodes/{NodeId} specifies getting node information for NodeId. If I were to create a url like this: /subscriptions/{SubId}/.../nodes/StatusCount or /subscriptions/{SubId}/.../nodes/NodeConfigurationCount then any user who tried to create a node with StatusCount or NodeConfigurationCount as a name would break our system. Possibly a small corner case but I didn't want to go there...
The count data is a different view of nodes where getting the status count is essentially a count of all nodes by status (think SQL group by) and getting the nodeConfiguration count is getting a count of all nodes with a particular node configuration value making the count data not a property of a node but a property of the entire collection of nodes. I modeled counts as a separate resource due to the above because all other approaches seemed wrong.
Signing off! |
@ravbhatnagar - thanks @mcardosos - could you merge? |
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.
I know this errors were not introduced on this PR, but, pointing them out for awareness
Hi @vrdmr please ping this thread when your ready to merge |
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.
New functionality to address server side (sql) paging for dsc resources
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