[DataBoxEdge] adding orders cmdlet and changes related to local share and adding iot roles #10527

raviten · 2019-11-13T00:49:35Z

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

design review

Had made change from TrackingInfo to ForwardTrackingInfo as suggested

Had made change of parameterset for share creation as it requires DataFormat in Cloud Share creation only

…t roles

adxsdkps · 2019-11-13T00:49:38Z

Can one of the admins verify this patch?

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderGetCmdlet.cs

erich-wang

@raviten , please update changelog.md as well.

erich-wang · 2019-11-15T03:24:28Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderGetCmdlet.cs

+                WriteObject(ListResource(), true);
+            }
+
+


nit: remove white lines

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderNewCmdlet.cs

erich-wang · 2019-11-15T03:34:09Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderNewCmdlet.cs

+                this.ResourceGroupName);
+        }
+
+        private string GetResourceNotFoundMessage()


Should be GetResourceAlreadyExistMessage?

This was consistent with other code structure, Is it fine if i raise a separate PR for this kind of change

I mean the method name is the opposite of what it really does, so you should change its name to 'GetResourceAlreadyExistMessage'

erich-wang · 2019-11-15T06:54:04Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Share/DataBoxEdgeShareNewCmdletBase.cs

@@ -143,14 +186,33 @@ private PSResourceModel CreateResourceModel()
                this.ResourceGroupName));
        }

+        private ResourceModel AddAzureContainer(ResourceModel resourceModel)
+        {
+            var sac = this.DataBoxEdgeManagementClient.StorageAccountCredentials.Get(


please use meaningful name instead of sac

Sure, will do

raviten · 2019-11-16T15:16:56Z

@raviten , please update changelog.md as well.

Sure, will do

…rderHistory as they will be part of output object only. Updated changelog md and sorted name spaces

erich-wang · 2019-11-17T10:22:53Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderGetCmdlet.cs

+using PSResourceModel = Microsoft.Azure.PowerShell.Cmdlets.DataBoxEdge.Models.PSDataBoxEdgeOrder;
+using PSTopLevelResource = Microsoft.Azure.PowerShell.Cmdlets.DataBoxEdge.Models.PSDataBoxEdgeDevice;
+using ResourceModel = Microsoft.Azure.Management.EdgeGateway.Models.Order;
+


Do not need to use alias for them, simply use original type name will be fine, same for other cmdlets.

erich-wang · 2019-11-17T10:24:28Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderGetCmdlet.cs

+            ParameterSetName = GetByDeviceObjectParameterSet,
+            HelpMessage = Constants.PsDeviceObjectHelpMessage)]
+        [ValidateNotNullOrEmpty]
+        public PSTopLevelResource DeviceObject { get; set; }


Please add

ValueFromPipeline = true

erich-wang · 2019-11-17T10:25:03Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderGetCmdlet.cs

+            HelpMessage = Constants.ResourceIdHelpMessage)]
+        [ValidateNotNullOrEmpty]
+        public string ResourceId { get; set; }
+


ValueFromPipelineByPropertyName = true

erich-wang · 2019-11-17T10:25:15Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderGetCmdlet.cs

+            Position = 0)]
+        [ValidateNotNullOrEmpty]
+        [ResourceGroupCompleter]
+        public string ResourceGroupName { get; set; }


ValueFromPipelineByPropertyName = true

erich-wang · 2019-11-17T10:25:25Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderGetCmdlet.cs

+            Position = 1)]
+        [ResourceNameCompleter("Microsoft.DataBoxEdge/dataBoxEdgeDevices", nameof(ResourceGroupName))]
+        [ValidateNotNullOrEmpty]
+        public string DeviceName { get; set; }


ValueFromPipelineByPropertyName = true

Please use Name as parameter name, and use DeviceName as alias. Same for other cmdlet.
Normally we prefer to use Name for current level resource.

For each and every Order's resource name is constant and value of order's name is default and is under parent resource DataBoxEdge's resource which is referred here as DeviceName.
Since taking Name as parameter is redundant skipped accepting Name as parameter but in the future for some reason name can be reused

In this case, please keep DeviceName

erich-wang · 2019-11-17T10:44:19Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderRemoveCmdlet.cs

+            HelpMessage = Constants.InputObjectHelpMessage
+        )]
+        [ValidateNotNull]
+        public PSResourceModel InputObject { get; set; }


ValueFromPipeline = true

erich-wang · 2019-11-17T10:44:27Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderRemoveCmdlet.cs

+            HelpMessage = Constants.ResourceGroupNameHelpMessage, Position = 0)]
+        [ValidateNotNullOrEmpty]
+        [ResourceGroupCompleter]
+        public string ResourceGroupName { get; set; }


ValueFromPipelineByPropertyName = true

erich-wang · 2019-11-17T10:44:34Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderRemoveCmdlet.cs

+        [Parameter(Mandatory = true, ParameterSetName = DeleteByNameParameterSet,
+            HelpMessage = Constants.DeviceNameHelpMessage, Position = 1)]
+        [ValidateNotNullOrEmpty]
+        public string DeviceName { get; set; }


ValueFromPipelineByPropertyName = true

erich-wang · 2019-11-17T10:45:07Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Roles/DataBoxEdgeRoleSetCmdlet.cs

+            ParameterSetName = SetByResourceIdParameterSet,
+            HelpMessage = Constants.ResourceIdHelpMessage)]
+        [ValidateNotNullOrEmpty]
+        public string ResourceId { get; set; }


ValueFromPipelineByPropertyName = true

erich-wang · 2019-11-17T10:45:14Z

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Roles/DataBoxEdgeRoleSetCmdlet.cs

+            Position = 0)]
+        [ValidateNotNullOrEmpty]
+        [ResourceGroupCompleter]
+        public string ResourceGroupName { get; set; }


ValueFromPipelineByPropertyName = true

adding orders cmdlet and changes related to local share and adding io…

b70467f

…t roles

resolving static analysis error

66dec65

raviten changed the title ~~adding orders cmdlet and changes related to local share and adding iot roles~~ [DataBoxEdge] adding orders cmdlet and changes related to local share and adding iot roles Nov 13, 2019

azuresdkci added the needs-review label Nov 13, 2019

isra-fel assigned erich-wang Nov 13, 2019

erich-wang reviewed Nov 15, 2019

View reviewed changes

src/DataBoxEdge/DataBoxEdge/Common/Cmdlets/Orders/DataBoxEdgeOrderGetCmdlet.cs Show resolved Hide resolved

erich-wang suggested changes Nov 15, 2019

View reviewed changes

erich-wang added needs-revision and removed needs-review labels Nov 15, 2019

erich-wang assigned raviten Nov 15, 2019

Ravi Teja added 2 commits November 16, 2019 21:00

removing switch parameters ForwardTrackingInfo, ReturnTrackingInfo, O…

3df40a8

…rderHistory as they will be part of output object only. Updated changelog md and sorted name spaces

renaming sac => storageAccountCredential

6771964

raviten requested a review from erich-wang November 16, 2019 15:36

raviten added needs-review and removed needs-revision labels Nov 16, 2019

erich-wang suggested changes Nov 17, 2019

View reviewed changes

incorporating review feedback

f50b80d

erich-wang approved these changes Nov 17, 2019

View reviewed changes

erich-wang merged commit 9565241 into Azure:master Nov 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataBoxEdge] adding orders cmdlet and changes related to local share and adding iot roles #10527

[DataBoxEdge] adding orders cmdlet and changes related to local share and adding iot roles #10527

raviten commented Nov 13, 2019

adxsdkps commented Nov 13, 2019

erich-wang left a comment

erich-wang Nov 15, 2019

erich-wang Nov 15, 2019

raviten Nov 16, 2019 •

edited

Loading

erich-wang Nov 17, 2019

erich-wang Nov 15, 2019

raviten Nov 16, 2019

raviten commented Nov 16, 2019

erich-wang Nov 17, 2019 •

edited

Loading

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

raviten Nov 17, 2019

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

erich-wang Nov 17, 2019

[DataBoxEdge] adding orders cmdlet and changes related to local share and adding iot roles #10527

[DataBoxEdge] adding orders cmdlet and changes related to local share and adding iot roles #10527

Conversation

raviten commented Nov 13, 2019

Description

Checklist

adxsdkps commented Nov 13, 2019

erich-wang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raviten Nov 16, 2019 • 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

raviten commented Nov 16, 2019

erich-wang Nov 17, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raviten Nov 16, 2019 •

edited

Loading

erich-wang Nov 17, 2019 •

edited

Loading