Add download range to Get-AzureBatchNodeFileContent #4198

matthchr · 2017-06-27T00:15:01Z

Description

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.

msftclas · 2017-06-27T00:15:05Z

@matthchr,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

matthchr · 2017-06-27T00:15:46Z

This is for issue #3863

matthchr · 2017-06-27T00:17:30Z

I have added a DO NOT MERGE label until one of my colleagues takes a look and signs off, at which point I will remove the label.

Azure PSH folks @cormacpayne @markcowl feel free to review now.

itowlson · 2017-06-27T01:05:33Z

src/ResourceManager/AzureBatch/Commands.Batch/Models/BatchClient.Files.cs

+            Protocol.RequestInterceptor interceptor = new Protocol.RequestInterceptor(baseRequest =>
+            {
+                var fromTaskRequest = baseRequest as BatchRequests.FileGetFromTaskBatchRequest;
+                if (fromTaskRequest != null && rangeStart != null && rangeEnd != null)


So if they specify a start but not an end, or vice versa... does this behave correctly? Finding it a bit hard to track if they get defaulted somewhere along the line, but since they are still nullable at this point I am kind of guessing not...?

Yeah it would behave correctly, but maybe should make it clearer that that's the case.

Basically the defaults work such that they can either both be set or neither is set. In the case where neither is set we do what we did before (don't include the range header). In the case where one is set, the other is defaulted (to 0 for start or to long.Max for End)

cormacpayne

@matthchr a few minor comments

cormacpayne · 2017-06-27T16:43:03Z

src/ResourceManager/AzureBatch/ChangeLog.md

@@ -21,7 +21,7 @@

 ## Version 4.1.0
 - Added new Get-AzureBatchJobPreparationAndReleaseTaskStatus cmdlet.
-
+- Added byte range start and end to Get-AzureBatchNodeFileContent parameters.


@matthchr this should go under the Current Release header

And fixed the one above too since they are both in the same release I think, it must've been missed in my previous PR

cormacpayne · 2017-06-27T16:46:06Z

src/ResourceManager/AzureBatch/Commands.Batch/Files/GetBatchNodeFileContentCommand.cs

+
+        [Parameter(HelpMessage = "The end of the byte range to be downloaded. If this is not specified and ByteRangeStart is, this defaults to the end of the file.")]
+        [ValidateNotNullOrEmpty]
+        public long? ByteRangeEnd { get; set; }


@matthchr does it make sense to use the ValidateRange attribute for these parameters so you can restrict the values passed through to these parameters? (i.e., negative values cannot be used)

Yea, good idea, I added this

cormacpayne · 2017-06-27T16:49:49Z

src/ResourceManager/AzureBatch/Commands.Batch/help/Get-AzureBatchNodeFileContent.md

@@ -130,6 +132,32 @@ Accept pipeline input: True (ByValue)
 Accept wildcard characters: False
 ```

+### -ByteRangeEnd
+The end of the byte range to be downloaded.```yaml


@matthchr platyPS does this weird thing where this ```yaml string is added at the end of the description rather than on the next line by itself, which can cause the markdown to be rendered incorrectly (like it does in this file). Would you mind moving all instances of this ```yaml string to a new line by itself?

matthchr · 2017-06-27T16:57:20Z

@itowlson I clarified that it's not possible for one of the ranges to be null while the other isn't in the function signature.

I also added a //TODO to move to use the new way of doing this once we upgrade our C# SDK version here.

itowlson · 2017-06-27T22:19:45Z

src/ResourceManager/AzureBatch/Commands.Batch.Test/Files/GetBatchNodeFileContentCommandTests.cs

+
+        [Fact]
+        [Trait(Category.AcceptanceType, Category.CheckIn)]
+        public void GetBatchNodeFileByteRange()


Test name should reflect the Fact that it claims to be true.

Fixed (it's a Theory now though)

itowlson · 2017-06-27T22:20:08Z

src/ResourceManager/AzureBatch/Commands.Batch.Test/Files/GetBatchNodeFileContentCommandTests.cs

+            cmdlet.InputObject = null;
+            cmdlet.DestinationPath = null;
+            cmdlet.ByteRangeStart = 7;
+            cmdlet.ByteRangeEnd = 14;


Are there tests for the 'only one end set' case?

matthchr · 2017-06-28T00:29:08Z

@cormacpayne @shahabhijeet - I think I have responded to all comments from my team and you as well. Please feel free to merge at this point provided things look good

shahabhijeet · 2017-06-28T05:21:37Z

On-Demand
http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1714/

matthchr · 2017-06-28T16:41:58Z

@cormacpayne Can you give this another look and merge if it looks good to you?

msftclas added the cla-not-required label Jun 27, 2017

matthchr requested review from itowlson, darylmsft and xingwu1 June 27, 2017 00:16

matthchr added the Do Not Merge 🚫 label Jun 27, 2017

azuresdkci added the needs-review label Jun 27, 2017

azuresdkci assigned shahabhijeet Jun 27, 2017

itowlson reviewed Jun 27, 2017

View reviewed changes

shahabhijeet removed the needs-review label Jun 27, 2017

shahabhijeet assigned matthchr Jun 27, 2017

cormacpayne suggested changes Jun 27, 2017

View reviewed changes

matthchr force-pushed the feature/download-ranges branch from 42a6cc9 to f7ff48f Compare June 27, 2017 16:56

matthchr force-pushed the feature/download-ranges branch from f7ff48f to da33891 Compare June 27, 2017 17:03

itowlson previously approved these changes Jun 27, 2017

View reviewed changes

Add download range to Get-AzureBatchNodeFileContent

921ca30

matthchr dismissed itowlson’s stale review via 921ca30 June 28, 2017 00:27

matthchr force-pushed the feature/download-ranges branch from da33891 to 921ca30 Compare June 28, 2017 00:27

matthchr removed the Do Not Merge 🚫 label Jun 28, 2017

shahabhijeet added the Waiting for On Demand label Jun 28, 2017

cormacpayne approved these changes Jun 28, 2017

View reviewed changes

cormacpayne merged commit fe5a16a into Azure:preview Jun 28, 2017

matthchr deleted the feature/download-ranges branch September 28, 2017 01:57

Add download range to Get-AzureBatchNodeFileContent #4198

Add download range to Get-AzureBatchNodeFileContent #4198

Uh oh!

Conversation

matthchr commented Jun 27, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

msftclas commented Jun 27, 2017

Uh oh!

matthchr commented Jun 27, 2017

Uh oh!

matthchr commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

matthchr commented Jun 27, 2017

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!

matthchr commented Jun 28, 2017

Uh oh!

shahabhijeet commented Jun 28, 2017

Uh oh!

matthchr commented Jun 28, 2017

Uh oh!

Uh oh!

matthchr commented Jun 27, 2017 •

edited by azuresdkci

Loading