Skip to content

Allow caller to Specify desired OutputType #499

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

Merged
merged 18 commits into from
Jan 27, 2021
Merged

Conversation

georgend
Copy link
Contributor

@georgend georgend commented Jan 13, 2021

Add -OutputType [HashTable | PSObject | HttpResponseMessage | Json] parameter to enable caller decide preferred format. HashTable is the default.
PSObject for a Single Entity
PsObject
PSObject for a collection
PsObjectArray
HttpResponseMessage
NewHttpReponseMessage

Resolves #480 #479 #454

Add "-Json" to return unprocess Json String
Add "-Raw -OriginalRequestClone "requestClone" " to store a clone of the original request in a variable, including its content.
@KirkMunro
Copy link

I think you need to simplify the variables that are used here, and fit them more into existing PowerShell paradigms.

For example, instead of:

  • returning a custom PowerShell object by default
  • returning the HttpResponseMessage if the -Raw switch is used
  • returning the JSON string if the -Json switch is used

Why not provide an -OutputType parameter that is an enumeration. The enumeration would have three values: PSObject, Json, and HttpResponseMessage. The default value would be PSObject. This would allow the cmdlets to work like they already do today, while allowing users to decide if they want the raw JSON or the HttpResponseMessage object, without having to deal with multiple parameters that are mutually exclusive of one another.

Also, for the -OriginalRequestClone "createRequestClone" parameter that creates a "magic" $requestClone variable in the current scope, that is not consistent with existing methods used in PowerShell is used to output values into a variable. For example, PowerShell supports common -OutVariable, -ErrorVariable, -WarningVariable, and -PipelineVariable parameters that are used to redirect various things into PowerShell variables. The user provides the variable name, and the content is assigned to that variable. The user can choose to overwrite the content of the variable, or add content to the variable. For instance, consider this script:

# Get some services, output them to the console, and store them in $myServices
gsv -name wuauserv,bits -ov myServices
# Show the services that were stored
$myServices
# Get some more services, and _add_ them to the myServices variable (note the "+" prefix)
gsv -name k* -ov +myServices
# Show the updated services collection
$myServices

This is the model that should be followed for requesting the HTTP request. If you were to apply this to the Graph cmdlets, the command might look like this:

# Get users, output them, and capture the HTTP request
Get-MgUser -HttpRequestVariable httpRequest
# Look at the request
$httpRequest

In this scenario, you are not dealing with data streams, so I don't think there is a lot of value in supporting the "+" prefix to add another request to an existing variable, other than to maintain consistency with the model that is already in place for capturing content in variables, so it's up to you whether or not you want to add that.

I think these two sets of changes would make for a much better experience for PowerShell scripters that is easier to learn and use.

@ddyett ddyett requested review from roinochieng and removed request for roinochieng January 14, 2021 18:52
@georgend georgend requested a review from peombwa January 14, 2021 23:15
@georgend georgend changed the title Expose Full Http Response and original Json String Allow caller to Specify desired OutputType Jan 14, 2021
George Ndungu added 2 commits January 20, 2021 20:30
Remove ternary operation due to implicit convertion of Hashtable to PSObject.
@ddyett
Copy link
Contributor

ddyett commented Jan 21, 2021

@finsharp do we have some limitation on the azure machines we need to look at. The errors for the integrated build seem to be indicating that.

George Ndungu added 4 commits January 22, 2021 19:27
George Ndungu and others added 2 commits January 23, 2021 02:38
Throw error correctly.
@georgend georgend merged commit b82bbab into dev Jan 27, 2021
@peombwa peombwa mentioned this pull request Jan 28, 2021
@peombwa peombwa deleted the features/IGRImprovements branch January 29, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants