Skip to content

invoke-azrest #12291

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 24 commits into from
Jul 1, 2020
Merged

invoke-azrest #12291

merged 24 commits into from
Jul 1, 2020

Conversation

VeryEarly
Copy link
Collaborator

Description

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@VeryEarly VeryEarly requested a review from dingmeng-xue June 30, 2020 03:30
@VeryEarly VeryEarly self-assigned this Jun 30, 2020
@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/Models/PSAzureOperationResponse.cs Outdated
@@ -0,0 +1,37 @@
using System;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license declaration.

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/Models/PSHttpResponse.cs Outdated
@@ -0,0 +1,32 @@
using System;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license declaration.

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/Rest/InvokeAzRestCommand.cs Outdated

namespace Microsoft.Azure.Commands.Profile.Rest
{
[Cmdlet("Invoke", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "Rest", DefaultParameterSetName = ByPath, SupportsShouldProcess = true), OutputType(typeof(PSHttpResponse))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace "Invoke" using PowerShell constant

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/Rest/InvokeAzRestCommand.cs Outdated
[ValidateNotNullOrEmpty]
public string Path { get; set; }

[Parameter(Mandatory = true, HelpMessage = "Api Version")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it mandatory? Can we allow user append API on query path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer leave this as an independent parameter

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/Rest/InvokeAzRestCommand.cs Outdated
{
if (this.IsParameterBound(c => this.ResourceType) && !this.IsParameterBound(c => this.Name))
{
throw new PSArgumentException("number of resource types and resource names must be the same");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember you have no plan to expose type and name pairs. If you still want to use it, it seems you need to add an additional parameter set to indicate resource type and name both need to be shown up. Then linke 152-160 are not required.

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/Rest/Utils.cs Outdated

namespace Microsoft.Azure.Commands.Profile.Rest
{
public class Utils
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge it into InvokeAzRestCommand because it is used by it for the time being.

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/help/Az.Accounts.md Outdated
@@ -72,8 +72,11 @@ Gets tenants that are authorized for the current user.
### [Import-AzContext](Import-AzContext.md)
Loads Azure authentication information from a file.

### [Invoke-AzRest](Invoke-AzRest.md)
{{ Fill in the Synopsis }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update it.

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/help/Invoke-AzRest.md Outdated
# Invoke-AzRest

## SYNOPSIS
Construct and perform HTTP request
Copy link
Member

@dingmeng-xue dingmeng-xue Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to emphasize the request to Azure resource management URL only

dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/help/Invoke-AzRest.md Outdated
}
```

Get LA Workspace by path
Copy link
Member

@dingmeng-xue dingmeng-xue Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LA is short for?

@dingmeng-xue dingmeng-xue linked an issue Jun 30, 2020 that may be closed by this pull request
Create a cmdlet to make a generic call to REST API (Invoke-AzRestMethod) #9440
Closed
dingmeng-xue
dingmeng-xue reviewed Jun 30, 2020
View reviewed changes
src/Accounts/Accounts/ChangeLog.md Outdated
@@ -18,6 +18,7 @@
- Additional information about change #1
-->
## Upcoming Release
* Added new cmdlet `Invoke-AzRest`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change it to Invoke-AzRestMethod?

@VeryEarly
Copy link
Collaborator Author

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@VeryEarly VeryEarly merged commit abf6f37 into Azure:master Jul 1, 2020
litchiyangMSFT pushed a commit to litchiyangMSFT/azure-powershell that referenced this pull request Aug 4, 2020
@VeryEarly @litchiyangMSFT
invoke-azrest (Azure#12291)
4ae2335 
* support generic get

* update

* update test case

* remove api version when construct uri

* update uri construct

* update localfeed

* to use full response

* convert output to PSHttpResponse

* remove request id from pshttpresponse

* rename to invoke-azrest

* update parameter name

* create help markdown for invoke-azrest

* update common API reference

* clean localfeed

* add test case for invoke-azrest

* update changelog.md

* resolve comments

* fix bug in get api version

* update changelog

* fix help

* handle upper case in path

* fix bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dingmeng-xue dingmeng-xue dingmeng-xue approved these changes

Assignees

@VeryEarly VeryEarly

Labels
needs-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a cmdlet to make a generic call to REST API (Invoke-AzRestMethod)
3 participants
@VeryEarly @dingmeng-xue @azuresdkci