Skip to content

Conversation

@blueww
Copy link
Member

@blueww blueww commented Jul 23, 2020

Description

Share Usage pass review in (part of the design): Azure/azure-powershell-cmdlet-review-pr#559

The code change has been in preview module before by part of the following 2 PRs:
#11779
#12013

Please note, I have change the parameter set of "Get-AzRmStorageShare", add add breaking change exception (align with the design).
But this change won't break any script, see the change reason in : https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/559#issuecomment-617209043
Please let me know if this change is not OK for stable PSH, I will revert to original parameter set , and add error message when user don't input correct parameter combination.

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

### ShareResourceId
```
Get-AzRmStorageShare [-ResourceId] <String> [-Name <String>] [-DefaultProfile <IAzureContextContainer>]
Get-AzRmStorageShare [-ResourceId] <String> [-GetShareUsage] [-DefaultProfile <IAzureContextContainer>]
Copy link
Contributor

Choose a reason for hiding this comment

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

ShareResourceId still will cause breaking changes for the users who both use -ResourceId and -Name. I think you should do the same thing with AccountObject.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@wyunchi-ms
Thanks for the catch!
Actually, parameterset "ShareResourceId" should not container "-Name", since resource ID already contains share name information, and my code always get share name from resource ID when use parameterset "ShareResourceId", "-Name" will be omit.
So it should be a bug to include "-Name" in parameterset "ShareResourceId".
Anyway, to avoid breaking change, I have add "-Name" back to parameterset "ShareResourceId", and give a warning that -Name will be omit.

Do you have any suggestion how to display breaking change notice only when use "-Name" in parameterset "ShareResourceId"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding the attribute CmdletParameterBreakingChange("Name", deprecateByVersion="3.0.0", ChangeDescription="Name info is already incloud by ShareResourceId, so it's not necessary any more.") is OK. What do you think?

Copy link
Member Author

@blueww blueww Jul 24, 2020

Choose a reason for hiding this comment

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

If use CmdletParameterBreakingChange, the breaking change notice will be show when use -Name, that might be too bother. I only want to show it when user use both -ResourceID and -Name.

If there any way to raise the breaking notice in cmdlet ExecuteCmdlet(), then I can raise the breaking notice after check user input both -ResourceID and -Name. (I think there will be very small amount of customer can be impact by the breaking, since this is not a very valid usage. So don't want to bother the normal user too much.)

@wyunchi-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@wyunchi-ms wyunchi-ms merged commit df75b37 into Azure:master Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants