-
Notifications
You must be signed in to change notification settings - Fork 45
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
Feature Get-vRARequestResult #167
Conversation
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.
This looks great, thanks for your contirbution - can you add some tests for this too? There are plenty of examples in the tests directory.
I've also added some comments to the review, could you have a look through them?
.OUTPUTS | ||
System.Management.Automation.PSObject | ||
|
||
.EXAMPLE |
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.
Add an example to show pipeline support:
Get-vRARequest -RequestNumber 339 | Get-vRARequestResult
@@ -0,0 +1,94 @@ | |||
|
|||
function Get-vRARequestResult { |
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.
Possible function rename? I feel that Get-vRARequestDetail would suit better as Get-vRARequest actually returns the result of the request.. @jonathanmedd Interested in your views
|
||
$id = (Get-vRARequest -RequestNumber $RequestNumber).id | ||
|
||
Get-vRARequestResult -id $id |
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.
Avoid this recursion if possible... just use the same pattern that you did in the 'ById' block 👍
Sorry for wrong actions on GitHub, I'm pretty new in this exercice :p |
|
||
$id = (Get-vRARequest -RequestNumber $RequestNumber).id | ||
|
||
Get-vRARequestDetail -id $id |
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.
Just this one left to change now.. i would do something similar to this (would need testing though):
$Id = (Get-vRARequest -RequestNumber $RequestNumber).id
$URI = "/catalog-service/api/consumer/requests/$($Id)/forms/details"
$RequestDetail = Invoke-vRARestMethod -Method GET -URI $URI -Verbose:$VerbosePreference
$RequestDetail.values.entries
@otabut No need to apologise! We appreciate your contribution. There's just one more change to make, which i've mentioned it above, then @jonathanmedd will then review it. This will go in to the next release! |
|
||
# --- If the id parameter is passed returned detailed information about the request | ||
'ById' { | ||
|
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.
We need to support supplying multiple Ids and RequestNumbers, so both of these sections need a foreach statement. Also I would smarten up the output a little bit. So I would do it like the following:
switch ($PsCmdlet.ParameterSetName) {
# --- If the id parameter is passed returned detailed information about the request
'ById' {
foreach ($RequestId in $Id){
$RequestNumber = (Get-vRARequest -Id $RequestId).RequestNumber
$URI = "/catalog-service/api/consumer/requests/$($RequestId)/forms/details"
$RequestDetail = Invoke-vRARestMethod -Method GET -URI $URI -Verbose:$VerbosePreference
[PSCustomObject] @{
Id = $RequestId
RequestNumber = $RequestNumber
Detail = $RequestDetail.values.entries
}
}
break
}
# --- If the request number parameter is passed returned detailed information about the request
'ByRequestNumber' {
foreach ($Number in $RequestNumber){
$RequestId = (Get-vRARequest -RequestNumber $Number).id
$URI = "/catalog-service/api/consumer/requests/$($RequestId)/forms/details"
$RequestDetail = Invoke-vRARestMethod -Method GET -URI $URI -Verbose:$VerbosePreference
[PSCustomObject] @{
Id = $RequestId
RequestNumber = $Number
Detail = $RequestDetail.values.entries
}
}
break
}
}
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 to me. If you just update it based on my comment, then I'd be happy to approve and merge
|
||
.EXAMPLE | ||
Get-vRARequest -RequestNumber 965299 | Get-vRARequestDetail | ||
|
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 you also add an example for supporting multiple Ids or numbers, e.g.
.EXAMPLE
Get-vRARequestDetail -RequestNumber 965299,965300
Enhancement
#165 - cmdlet for catching request result by invoking /catalog-service/api/consumer/requests//forms/details
Also need to modify PowervRA.psd1