[Azure.Storage] Support defaultserviceversion #5219

blueww · 2018-01-04T08:29:26Z

Description

This PR will support set/get Default service version in Powershell cmdlet.
The design is reviewed in https://github.com/Azure/azure-powershell-pr/wiki/Storage-Blob-Service-Default-Service-Version.

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

maddieclayton

Please add tests and I'll take a look.

maddieclayton · 2018-01-05T21:39:20Z

src/Storage/Azure.Storage.psd1

@@ -115,7 +115,9 @@ CmdletsToExport = 'Get-AzureStorageTable', 'New-AzureStorageTableSASToken',
               'Set-AzureStorageContainerStoredAccessPolicy', 


Please update the changelog with these changes.

change log is added

maddieclayton · 2018-01-05T21:41:05Z

src/Storage/Azure.Storage.psd1

@@ -115,7 +115,9 @@ CmdletsToExport = 'Get-AzureStorageTable', 'New-AzureStorageTableSASToken',
               'Set-AzureStorageContainerStoredAccessPolicy', 
               'Start-AzureStorageBlobCopy', 
               'Start-AzureStorageBlobIncrementalCopy', 


Please add tests for your new cmdlets.

As the Storage Client library request can't be record in Unit test framework, generally we don't add unit test in PowerShell solution for data plan cmdelts. We will add functional test for the new cmdelts in storage team regression test. Let me know if you would like to review the CR for the our team regression test.

I'd love to review the regression tests before this change goes into our repo.

Hi Maddie,

Following is the CR link for the test cases. I have sent you another mail for the CR and attached the test log. You can see the powershell cmdelts tested and result validation of the test cases in the test log.

codeflow://open/?server=https%3A%2F%2Fmsazure.visualstudio.com%2F&review=One.664474&host=vso&alert=true

@maddieclayton , @cormacpayne

I have added the Unit test for the service properties parse function as we discussed into the PR.
Please help to review.

blueww · 2018-01-09T06:13:41Z

on-demand test updated: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/331/

maddieclayton · 2018-01-12T01:40:54Z

@blueww Cormac will take a look at this PR now, as he did the cmdlet review.

blueww · 2018-01-15T10:38:55Z

@cormacpayne
The PR has been filed for several days, and the last fix is 6 days ago.
Since the Code complete is near, would you please help to take a look to reduce the risk?
Hope we can make it in ASAP.

cormacpayne

@blueww a few minor comments that need to be addressed

cormacpayne · 2018-01-16T22:07:51Z

src/Storage/Commands.Storage/help/Get-AzureStorageServiceProperty.md

+
+### Example 1: Get  Azure Storage services property of the Blob service
+```
+C:\PS>Get-AzureStorageServiceProperty -ServiceType Blob


@blueww would you mind providing the output from this command as well so users have an idea of what will be returned?

The output is provided

cormacpayne · 2018-01-16T22:08:18Z

src/Storage/Commands.Storage/help/Set-AzureStorageServiceProperty.md

+
+### Example 1: Set Blob Service DefaultServiceVersion to 2017-04-17
+```
+C:\PS>Set-AzureStorageServiceProperty -ServiceType Blob -DefaultServiceVersion 2017-04-17


@blueww same comment about providing the corresponding output from running this command

As we talked in the meeting, since we don't have output by default (output serviceProperties when user add -passthru, since this serviceProperties is set to server not get from server), so it's OK not add the output here.

cormacpayne · 2018-01-16T22:13:27Z

src/Storage/Commands.Storage/Common/Cmdlet/SetAzureStorageServiceProperties.cs

+    /// <summary>
+    /// Modify Azure Storage service properties
+    /// </summary>
+    [Cmdlet(VerbsCommon.Set, StorageNouns.StorageServiceProperty, SupportsShouldProcess = true), OutputType(typeof(PSSeriviceProperties))]


@blueww what do you think about changing the name of this cmdlet to Update-* instead of Set-*? It seems like when more parameters are added to this cmdlet, only the values of properties provided should be updated and the other values should remain the same.

The Update verb is usually reserved for PATCH commands, which is what I think you would want to do in the future with this cmdlet, and the Set verb is usually reserved for PUT commands. Let me know what you think.

Has updated the cmdelts name to Update-AzureStorageServiceProperty, help and changelog also updated.

…roperty, and fix help

blueww · 2018-01-17T07:44:14Z

on-demand in: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/360/

blueww · 2018-01-18T01:56:38Z

@cormacpayne
I have fixed all comments in the PR, and the PR has all check passed (include one demand test).
Would you please help to review and merge it ASAP, since it's very near to code complete.
Thanks!

blueww · 2018-01-19T09:55:17Z

@cormacpayne
Please help to review and merge the PR as tomorrow is code complete.
Thanks!

Resolved

azuresdkci added the needs-review label Jan 4, 2018

azuresdkci assigned cormacpayne Jan 4, 2018

maddieclayton assigned maddieclayton and unassigned cormacpayne Jan 4, 2018

maddieclayton previously requested changes Jan 5, 2018

View reviewed changes

maddieclayton assigned blueww and unassigned maddieclayton Jan 5, 2018

maddieclayton added needs-revision and removed needs-review labels Jan 5, 2018

maddieclayton assigned maddieclayton and unassigned blueww Jan 8, 2018

maddieclayton added needs-review and removed needs-revision labels Jan 8, 2018

maddieclayton assigned cormacpayne and unassigned maddieclayton Jan 9, 2018

blueww force-pushed the defaultserviceversion branch from aa596fd to 3f4c69a Compare January 9, 2018 06:11

blueww force-pushed the defaultserviceversion branch from 3f4c69a to c821f27 Compare January 10, 2018 08:20

maddieclayton assigned maddieclayton and cormacpayne and unassigned cormacpayne and maddieclayton Jan 10, 2018

blueww added 2 commits January 12, 2018 09:57

support defaultserviceversion

6f0d828

Add Unit test for the Storage ServiceProperties parse functions.

e7acd29

cormacpayne suggested changes Jan 16, 2018

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Jan 16, 2018

cormacpayne removed their assignment Jan 16, 2018

cormacpayne assigned blueww Jan 16, 2018

Change Set-AzureStorageServiceProperty to Update-AzureStorageServiceP…

ee0abd3

…roperty, and fix help

blueww force-pushed the defaultserviceversion branch from c821f27 to ee0abd3 Compare January 17, 2018 06:52

maddieclayton assigned cormacpayne and unassigned blueww Jan 17, 2018

maddieclayton added needs-review and removed needs-revision labels Jan 17, 2018

cormacpayne approved these changes Jan 19, 2018

View reviewed changes

cormacpayne merged commit 2f68a6b into Azure:preview Jan 19, 2018

		@@ -115,7 +115,9 @@ CmdletsToExport = 'Get-AzureStorageTable', 'New-AzureStorageTableSASToken',
		'Set-AzureStorageContainerStoredAccessPolicy',

[Azure.Storage] Support defaultserviceversion #5219

[Azure.Storage] Support defaultserviceversion #5219

Uh oh!

Conversation

blueww commented Jan 4, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

maddieclayton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueww Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueww commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maddieclayton commented Jan 12, 2018

Uh oh!

blueww commented Jan 15, 2018

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueww commented Jan 17, 2018

Uh oh!

blueww commented Jan 18, 2018

Uh oh!

blueww commented Jan 19, 2018

Uh oh!

Uh oh!

blueww commented Jan 4, 2018 •

edited by azuresdkci

Loading

blueww Jan 10, 2018 •

edited

Loading

blueww commented Jan 9, 2018 •

edited

Loading