Skip to content

Switch to SDK use in ResourceGroup Deployments cmdlets #12141

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 4 commits into from
Jun 18, 2020
Merged

Switch to SDK use in ResourceGroup Deployments cmdlets #12141

merged 4 commits into from
Jun 18, 2020

Conversation

Xynoclafe
Copy link
Contributor

@Xynoclafe Xynoclafe commented Jun 13, 2020 •
edited
Loading

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

@Xynoclafe Xynoclafe requested a review from filizt June 13, 2020 00:15
@adxsdkps
Copy link
Collaborator

Can one of the admins verify this patch?

@VeryEarly VeryEarly self-assigned this Jun 13, 2020
VeryEarly
VeryEarly requested changes Jun 13, 2020
View reviewed changes
Copy link
Collaborator

@VeryEarly VeryEarly left a comment

Choose a reason for hiding this comment

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

  • the new ouput type doesn't seem to match the cmdlet output type

  • please add changelog

@filizt
Copy link
Contributor

filizt commented Jun 14, 2020 

[Cmdlet("Get", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "ResourceGroupDeploymentOperation"), OutputType(typeof(PSObject))]

This needs to be PsDeploymentOperation since we're not returning the generic PS object anymore.

Refers to: src/Resources/ResourceManager/Implementation/ResourceGroupDeployments/GetAzureResourceGroupDeploymentOperationCmdlet.cs:30 in 603df06. [](commit_id = 603df06, deletion_comment = False)

@filizt
Copy link
Contributor

filizt commented Jun 14, 2020

Looks like we don't need this parameter any more. Could you add a deprecation attribute noting that it will be retired?

Refers to: src/Resources/ResourceManager/Implementation/ResourceGroupDeployments/GetAzureResourceGroupDeploymentOperationCmdlet.cs:47 in 603df06. [](commit_id = 603df06, deletion_comment = False)

@filizt
Copy link
Contributor

filizt commented Jun 14, 2020 

[Cmdlet("Get", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "ResourceGroupDeploymentOperation"), OutputType(typeof(PSObject))]

Is there any test coverage for this cmdlet? If not, it might be good if we add one.

Refers to: src/Resources/ResourceManager/Implementation/ResourceGroupDeployments/GetAzureResourceGroupDeploymentOperationCmdlet.cs:30 in 603df06. [](commit_id = 603df06, deletion_comment = False)

@Xynoclafe
Copy link
Contributor Author 

[Cmdlet("Get", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "ResourceGroupDeploymentOperation"), OutputType(typeof(PSObject))]

Is there any test coverage for this cmdlet? If not, it might be good if we add one.

Refers to: src/Resources/ResourceManager/Implementation/ResourceGroupDeployments/GetAzureResourceGroupDeploymentOperationCmdlet.cs:30 in 603df06. [](commit_id = 603df06, deletion_comment = False)

We already have test coverage for this cmdlet.

VeryEarly
VeryEarly requested changes Jun 16, 2020
View reviewed changes
Copy link
Collaborator

@VeryEarly VeryEarly left a comment

Choose a reason for hiding this comment

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

please regenerate help due to output type change

@Xynoclafe
Copy link
Contributor Author

please regenerate help due to output type change

The help file already reflects the new output type

@Xynoclafe
Copy link
Contributor Author

Xynoclafe commented Jun 16, 2020 •
edited
Loading

Is this considered a breaking change?

@VeryEarly
Copy link
Collaborator

please regenerate help due to output type change

The help file already reflects the new output type

Output type had been changed and is still PSObject in help:
https://github.com/Xynoclafe/azure-powershell/blob/xynoclafe/deploymentsSDKfix/src/Resources/Resources/help/Get-AzResourceGroupDeploymentOperation.md

@filizt
Copy link
Contributor

filizt commented Jun 17, 2020

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@VeryEarly VeryEarly merged commit 1e79c35 into Azure:master Jun 18, 2020
dingmeng-xue pushed a commit that referenced this pull request Jun 20, 2020
@Xynoclafe @dingmeng-xue
Switch to SDK use in ResourceGroup Deployments cmdlets (#12141)
d4fa13d 
* Switch to SDK use in ResourceGroup Deployments cmdlets

* Include review fixes

* Changed help file to reflect new output type
Xynoclafe added a commit to Xynoclafe/azure-powershell that referenced this pull request Jun 24, 2020
@Xynoclafe
Rollback breaking changes from PR Azure#12141
e8556bd
Xynoclafe added a commit to Xynoclafe/azure-powershell that referenced this pull request Jun 24, 2020
@Xynoclafe
Rollback breaking changes from PR Azure#12141
f042cdf
@Xynoclafe Xynoclafe mentioned this pull request Jun 24, 2020
Merged
8 tasks
VeryEarly pushed a commit that referenced this pull request Jun 29, 2020
@Xynoclafe
Rollback breaking changes from #12141 (#12260)
a1e52c1 
* Rollback breaking changes from PR #12141

* Fix changelog

* Fix changelog
msJinLei pushed a commit that referenced this pull request Sep 3, 2020
@Xynoclafe
Remove ApiVersion Parameter and change Get-AzResourceGroupOperation i…
ddaee47 
…mplementation to use SDK (#12642)

* Remove ApiVersion Parameter from deployments

* update changelog

* Delete ApiVersion param

* Switch to SDK use in ResourceGroup Deployments cmdlets

* Include review fixes

* Changed help file to reflect new output type

* Rollback breaking changes from PR #12141

* Change Get-AzResourceGroupDeploymentOperation implementation to use SDK

* Update help

* Rollback changes to help files of untouched cmdlets

* Suppress breaking change issues

* remove subscriptionId param

* Update breakingchanges csv

* Suppress breaking changes
isra-fel added a commit that referenced this pull request Oct 16, 2020
@Xynoclafe @isra-fel
Breaking Change - Removed ApiVersion parameter from Az.Resources depl…
1cfab69 
…oyments cmdlets and changed the output type of Get-AzResourceGroupDeploymentOperations (#13112)

* Remove ApiVersion Parameter from deployments

* update changelog

* Delete ApiVersion param

* Switch to SDK use in ResourceGroup Deployments cmdlets

* Include review fixes

* Changed help file to reflect new output type

* Rollback breaking changes from PR #12141

* Change Get-AzResourceGroupDeploymentOperation implementation to use SDK

* Update help

* Rollback changes to help files of untouched cmdlets

* Suppress breaking change issues

* remove subscriptionId param

* Update breakingchanges csv

* Suppress breaking changes

* Review fixes + breaking change suppression

* Add missed breaking change issue to fix static analysis failure

* Update ChangeLog.md

Co-authored-by: Yeming Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VeryEarly VeryEarly VeryEarly approved these changes

@filizt filizt Awaiting requested review from filizt

Assignees

@VeryEarly VeryEarly

Labels
needs-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Xynoclafe @adxsdkps @filizt @VeryEarly @azuresdkci