Migrate Restore-AzStorageBlobRange and ManagementPolicy related cmdlets #14

yifanz0 · 2022-05-30T07:53:59Z

Description

Migrate Restore-AzStorageBlobRange and ManagementPolicy related cmdlets

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

yaxia · 2022-05-31T03:35:16Z

src/Storage/Storage.Management/StorageAccount/RemoveAzureStorageAccountManagementPolicy.cs

 using System.Management.Automation;
+using Azure;


Put system and others assemblies in groups. VS sort-usings may help.

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

yaxia · 2022-05-31T03:37:55Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+                    }
+                    else
+                    {
+                        WriteWarning(string.Format("Restore blob ranges with Id  started. Restore blob ranges time to complete is dependent on the size of the restore."));


duplicated spaces

yaxia · 2022-05-31T03:39:43Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+                    }
+                    else
+                    {
+                        WriteWarning(string.Format("Restore blob ranges with Id  started. Restore blob ranges time to complete is dependent on the size of the restore."));


may be "without id", since restoreId is not fetched?

Maybe we should remove "with Id" here. OR jut report error, since it should have the restore ID.

Per discussion, removed "with id", and simply print a warning of "cannot fetch id" here.

yaxia · 2022-05-31T03:46:57Z

src/Storage/Storage.Management/StorageAccount/StorageAccountBaseCmdlet.cs

+            Uri queueEndpoint = storageAccountResource.Data.PrimaryEndpoints.Queue != null ? new Uri(storageAccountResource.Data.PrimaryEndpoints.Queue) : null;
+            Uri tableEndpoint = storageAccountResource.Data.PrimaryEndpoints.Table != null ? new Uri(storageAccountResource.Data.PrimaryEndpoints.Table) : null;
+            Uri fileEndpoint = storageAccountResource.Data.PrimaryEndpoints.File != null ? new Uri(storageAccountResource.Data.PrimaryEndpoints.File) : null;
+            string key = storageAccountResource.GetKeys().Value.Keys[0].Value;


check: storageAccountResource.GetKeys()?.Value?.Keys != null && storageAccountResource.GetKeys().Value.Keys.Count > 0

yaxia · 2022-05-31T03:50:41Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+        public static IList<Track2Models.BlobRestoreRange> ParseBlobRestoreRanges(PSBlobRestoreRange[] ranges)
+        {
+            IList<Track2Models.BlobRestoreRange> re = new List<Track2Models.BlobRestoreRange>();
+            if (ranges == null)


Do you want to make sure there is at least one item in the list? If so, you need to handle to case when ranges contains 0 item as well.

Per the PSH design, if user not input "-BlobRestoreRange", will restore all ranges (from " to "" ).
If user input the parameter "-BlobRestoreRange", normally willl input at least 1 range, and we will restore the ranged user specified.
However, there're special way to input an array with 0 range. If this happen, we will just restore with 0 ranges as user input. We have not get customer complain for this behavior before.

blueww · 2022-05-31T07:09:57Z

src/Storage/Storage.Management/Models/PSDataPolicy.cs

+            Track2.Models.DateAfterModification dateAfterModification = new Track2.Models.DateAfterModification();
+            dateAfterModification.DaysAfterLastAccessTimeGreaterThan = this.DaysAfterLastAccessTimeGreaterThan;
+            dateAfterModification.DaysAfterModificationGreaterThan = this.DaysAfterModificationGreaterThan;
+


missed DaysAfterLastTierChangeGreaterThan

If SDK still not support it, please add a TODO comment for it, and add the left item to the progress track excel.

Done. It's not supported yet, so added a TODO comment for it

src/Storage/Storage.Management/Models/PSDataPolicy.cs

blueww · 2022-05-31T08:54:37Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

@@ -12,15 +12,17 @@
 // limitations under the License.
 // ----------------------------------------------------------------------------------

+using Track2 = Azure.ResourceManager.Storage;
+using Track2Models = Azure.ResourceManager.Storage.Models;


Should update Storage.Management.csproj, by remove following line to include the file in az.storage.

<Compile Remove="StorageAccount\RestoreAzStorageBlobRange.cs" />

blueww · 2022-05-31T09:15:50Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

                    }
-                    catch (System.AggregateException ex) when (ex.InnerException is CloudException)
+                    catch (Exception ex)


Please catch a specific type exception, instead of all exceptions.

blueww · 2022-05-31T09:54:08Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

-                    if (status != null && status.Status == BlobRestoreProgressStatus.Failed)
+                    Track2.StorageAccountResource account = this.StorageClientTrack2.GetStorageAccount(this.ResourceGroupName, this.StorageAccountName);
+                    var restoreLro = account.RestoreBlobRanges(
+                        WaitUntil.Completed,


Here might should be "WaitUntil.Started", as it's for when waitForComplete is not specified .

blueww · 2022-05-31T09:57:22Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+                        new Track2Models.BlobRestoreContent(
+                            this.TimeToRestore.ToUniversalTime(),
+                            ParseBlobRestoreRanges(this.BlobRestoreRange)));
+                    if (restoreLro.Value != null && restoreLro.Value.Status != null && restoreLro.Value.Status == Track2Models.BlobRestoreProgressStatus.Failed)


This line of code won't work. As can't parse the restore status from the return value when use "WaitUntil.Started".
You might should make the parse rawresponds code to a function, and use it in both place.

Thanks for catching this! Fixed the implementation to fetch all the information we need from the raw response and create a BlobRestoreStatus.

blueww · 2022-05-31T09:58:24Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+        /// <summary>
+        /// Parse from PSBlobRestoreRange list to BlobRestoreRange list
+        /// </summary>
+        public static IList<Track2Models.BlobRestoreRange> ParseBlobRestoreRanges(PSBlobRestoreRange[] ranges)


This should be a function of class PSBlobRestoreRange.

Done. Didn't notice there's already a ParseBlobRestoreRanges in PSBlobRestoreRange. Therefore simply removing this duplicated function here.

yaxia · 2022-06-06T08:10:57Z

src/Storage/Storage.Management/Models/PSDataPolicy.cs

-                Actions = this.Actions is null ? null : this.Actions.ParseManagementPolicyAction(),
-                Filters = this.Filters is null ? null : this.Filters.ParseManagementPolicyFilter()
-            };
+            Track2.Models.ManagementPolicyAction actions = this.Actions is null ? null : this.Actions.ParseManagementPolicyAction();


Try the syntactic sugar: actions = this.Actions?.ParseManagementPolicyAction();

yaxia · 2022-06-06T08:11:39Z

src/Storage/Storage.Management/Models/PSDataPolicy.cs

-            };
+            Track2.Models.ManagementPolicyAction actions = this.Actions is null ? null : this.Actions.ParseManagementPolicyAction();
+            Track2.Models.ManagementPolicyDefinition policyDefinition = new Track2.Models.ManagementPolicyDefinition(actions);
+            policyDefinition.Filters = this.Filters is null ? null : this.Filters.ParseManagementPolicyFilter();


policyDefinition.Filters = this.Filters?.ParseManagementPolicyFilter();

yaxia · 2022-06-06T08:19:39Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+                    Dictionary<string, object> temp = restoreLro.GetRawResponse().Content.ToObjectFromJson() as Dictionary<string, object>;
+                    object restoreId;
+
+                    if (temp != null && temp.TryGetValue("restoreId", out restoreId))


We may return early when this condition is hit, otherwise all the places using temp.*** need a null-check.

Discussed offline with Wei and changed to throw an exception when temp is null. Thus temp referenced below is definitely not null.
Getting the raw response is a temporary workaround for this SDK issue. We will remove this chunk of code once the issue is fixed, and we should be able to get BlobRestoreStatus object directly with the SDK.

yaxia · 2022-06-06T08:20:40Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

                    }
+
+                    temp.TryGetValue("restoreId", out object restoreId);


temp.*** is not guarded by the null-check.

Changed to throw an exception when temp is null

yaxia · 2022-06-06T08:21:50Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+
+                        PSBlobRestoreRange blobRestoreRange = new PSBlobRestoreRange
+                        {
+                            StartRange = startRange == null ? null : startRange.ToString(),


startRange?.ToString() #Closed

same comment to the below

src/Storage/Storage.Management/Storage.Management.csproj

blueww · 2022-06-07T10:46:06Z

src/Storage/Storage.Management/Models/PSDataPolicy.cs

-        }
-        public DateAfterCreation ParseDateAfterCreation()
-        {
-            return new DateAfterCreation(this.DaysAfterCreationGreaterThan, this.DaysAfterLastTierChangeGreaterThan);


Same comment should be add to code for SDK still not support "DaysAfterLastTierChangeGreaterThan". Like // TODO: DaysAfterLastTierChangeGreaterThan is still not supported by SDK, will add later.

src/Storage/Storage.Management/StorageAccount/StorageAccountBaseCmdlet.cs

src/Storage/Storage.Management/Storage.Management.csproj

yaxia · 2022-06-08T09:51:50Z

src/Storage/Storage.Management/StorageAccount/StorageAccountBaseCmdlet.cs

+            if (storageAccountResource.GetKeys()?.Value?.Keys != null && storageAccountResource.GetKeys().Value.Keys.Count > 0)
+            {
+                key = storageAccountResource.GetKeys().Value.Keys[0].Value;
+            } else


else

Minor: put else in a new line

yaxia · 2022-06-08T09:54:37Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+                    PSBlobRestoreParameters blobRestoreParameters = new PSBlobRestoreParameters();
+                    Dictionary<string, object> paramMap = parameters as Dictionary<string, object>;
+
+                    paramMap.TryGetValue("timetoRestore", out object timeToRestore);


paramMap

could paraMap be null? #Closed

Fixed. Assign null to blobRestoreParameters when paramMap is null

yaxia · 2022-06-08T09:59:56Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+                    Dictionary<string, object> paramMap = parameters as Dictionary<string, object>;
+
+                    paramMap.TryGetValue("timetoRestore", out object timeToRestore);
+                    DateTimeOffset.TryParse(timeToRestore.ToString(), out DateTimeOffset parseDate);


timeToRestore

When paramMap.TryGetValue fails (e.g.: there is no "timetoRestore" field), the object is null. #Closed

yaxia · 2022-06-08T10:00:30Z

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs

+                    PSBlobRestoreParameters blobRestoreParameters = new PSBlobRestoreParameters();
+                    Dictionary<string, object> paramMap = parameters as Dictionary<string, object>;
+
+                    paramMap.TryGetValue("timetoRestore", out object timeToRestore);


timetoRestore

is this case sensitive? Should it be timeToRestore?

Double checked the response. Weird but it's timetoRestore (might be a typo on the server side?), so I'm keeping timetoRestore here.

yaxia

* init add powershell cmdlet * add customization for filedetails * correct readme to include file workspace create * add custom cmdlet for creating and uploading the file * chunking logic for larger files * add error for custom cmdlet when file too big * combined cmdlets * edit custom file upload command to not use default subscriptionid * no default value for subid in custom file upload * separate subscription and no subscription commands * remove comments from combined cmd for file * fix cmd * custom no subscription file upload commands * autogen docs for no sub file upload * hide individual file commands * add back workspace commands * add name as alias for fileworkspacename fileworkspacesnosubscription * remove update files and add alias for nosubscription file commands * tests for get service * tests for problem classification * try adding erroractionpreference = stop * tests for new file workspace * tests for get file workspace * more tests * add tests for new file and upload, removefile name as a parameter * remove new files no subscription * tests for get file * erroraction stop * remove unnecessary comments and print statements * docs for get support service * problem classification docs * add titles * documentation for file workspace commands * documentation for file/file workspace cmdlets * documentation for checkNameAvailability * added tests and documentation for operations, support ticket, communication and chat transcript cmdlets * added examples for operations, support ticket, communication and chat transcript cmdlets * resolve merge conflicts * resolved PR comments * get conflict file changes from grhuang/azsupport-autorest * resolved merge conflict * removed update communication sub and no sub scenarios * Revert "removed update communication sub and no sub scenarios" This reverts commit ccbfdee. * removed update communication sub and no sub scenarios , updated readme * make communiation and support ticket properties required in readme, edit get operation file name, edit calling internal cmdlets for file upload * fix documentation * fix top query * add custom error handler * consolidate list and get communicationsnosubscription and chattranscriptsnosubscription * init changes to allow no subscription recording tests-need to use csp partner account in tenant 2e6a0c9f-986d-480e-ad4b-bdfddc047aba * changes to not create new resources in playback * remove custom error handler csharp * init add changes to split subscription and no subscription tests * update skip * Update recordings * update recordings * add more examples to new-azsupportticket documentation * update documentation and readme * add directive back in * make advanced diagnostic consent required * Add custom error handling for New- and Update- cmdlets to print full details of error (#9) * Add custom error handler * Add default filter to retrieve tickets from the past week for Get-AzSupportTickets and Get-AzSupportTicketsNoSubscription (#10) * add custom filtering for list support ticket if no filter is applied * make transformations in swagger in readme to make enrollment id not readonly and show isTempTicket (#11) * Add argument completer (#12) * add argument completers * add quotes to argument completers * remove repeated time zone * Regenerate powershell module with GA swagger (#13) * Rerecord tests using GA version (#14) * add tests for file size (will add recordings once Ga swagger available) * Regenerate powershell module with GA swagger * Rerecord tests with GA version * Fix documentation * fix url for file upload --------- Co-authored-by: Grace Huang (from Dev Box) <[email protected]> Co-authored-by: grhuangmsft <[email protected]> Co-authored-by: Shreya Kumar <[email protected]> Co-authored-by: Yunchi Wang <[email protected]>

yifanz7 added 5 commits May 17, 2022 10:32

track2 initial setup and StorageAccount related cmdlets

3e1c47f

fix bugs and formatting per comments

c5fc86e

Fix AccountIdentityType

1abf7fe

migration for restore blob range, managementpolicy related cmdlets

412d6c4

asd

56168fa

yifanz0 requested a review from blueww May 30, 2022 07:53

yaxia reviewed May 31, 2022

View reviewed changes

src/Storage/Storage.Management/StorageAccount/RestoreAzStorageBlobRange.cs Outdated Show resolved Hide resolved

yaxia reviewed May 31, 2022

View reviewed changes

blueww reviewed May 31, 2022

View reviewed changes

src/Storage/Storage.Management/Models/PSDataPolicy.cs Show resolved Hide resolved

blueww reviewed May 31, 2022

View reviewed changes

yifanz7 added 2 commits June 6, 2022 10:36

fix per comments

02a29df

Add a new constrcutor in PSBlobRestore

ae29774

yaxia reviewed Jun 6, 2022

View reviewed changes

fix per comments

f31835f

blueww reviewed Jun 7, 2022

View reviewed changes

src/Storage/Storage.Management/Storage.Management.csproj Show resolved Hide resolved

blueww reviewed Jun 7, 2022

View reviewed changes

src/Storage/Storage.Management/StorageAccount/StorageAccountBaseCmdlet.cs Show resolved Hide resolved

add comments

dd5e036

blueww reviewed Jun 8, 2022

View reviewed changes

src/Storage/Storage.Management/Storage.Management.csproj Show resolved Hide resolved

fix per comments

fcfd28c

blueww approved these changes Jun 8, 2022

View reviewed changes

yaxia reviewed Jun 8, 2022

View reviewed changes

put parser into helper

43db5f3

yaxia approved these changes Jun 9, 2022

View reviewed changes

move common code outside of if-else

0fc210d

blueww approved these changes Jun 9, 2022

View reviewed changes

yifanz0 merged commit d858c2d into wastoresh:track2review Jun 10, 2022

Migrate Restore-AzStorageBlobRange and ManagementPolicy related cmdlets #14

Migrate Restore-AzStorageBlobRange and ManagementPolicy related cmdlets #14

Conversation

yifanz0 commented May 30, 2022 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaxia May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaxia May 31, 2022 • edited Loading

Choose a reason for hiding this comment

blueww May 31, 2022 • edited Loading

Choose a reason for hiding this comment

blueww May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueww May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaxia Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaxia Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaxia Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaxia Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaxia left a comment

Choose a reason for hiding this comment

yifanz0 commented May 30, 2022 •

edited

Loading

yaxia May 31, 2022 •

edited

Loading

yaxia May 31, 2022 •

edited

Loading

blueww May 31, 2022 •

edited

Loading

blueww May 31, 2022 •

edited

Loading

blueww May 31, 2022 •

edited

Loading

yaxia Jun 6, 2022 •

edited

Loading

yaxia Jun 8, 2022 •

edited

Loading

yaxia Jun 8, 2022 •

edited

Loading

yaxia Jun 8, 2022 •

edited

Loading