Service Bus: some issues fix and piping for remove cmdlet #6732

v-Ajnava · 2018-07-23T22:06:19Z

Description

added Piping scenario for remove cmdlets
fixed issues:
#4340
#3780

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.
[ X] 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 PassThrough 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.

…nto preview_ipobject # Conflicts: # src/ResourceManager/ServiceBus/Commands.ServiceBus/help/Remove-AzureRmServiceBusNamespace.md # src/ResourceManager/ServiceBus/Commands.ServiceBus/help/Remove-AzureRmServiceBusQueue.md # src/ResourceManager/ServiceBus/Commands.ServiceBus/help/Remove-AzureRmServiceBusRule.md # src/ResourceManager/ServiceBus/Commands.ServiceBus/help/Remove-AzureRmServiceBusSubscription.md # src/ResourceManager/ServiceBus/Commands.ServiceBus/help/Remove-AzureRmServiceBusTopic.md

cormacpayne

@v-Ajnava some comments for you to take a look at

cormacpayne · 2018-07-23T23:09:44Z

src/ResourceManager/ServiceBus/Commands.ServiceBus.Test/Commands.ServiceBus.Test.csproj

@@ -188,6 +188,7 @@
    <None Include="SessionRecords\Microsoft.Azure.Commands.ServiceBus.Test.ScenarioTests.ServiceBusDRConfigurationTests\ServiceBusDRConfigurationsCURD.json">
      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
    </None>
+    <None Include="SessionRecords\Microsoft.Azure.Commands.ServiceBus.Test.ScenarioTests.ServiceBusDRConfigurationTests\ServiceBusDRConfigurationsCURDAlterName.json" />


@v-Ajnava you'll need to make sure this is copied to the output directory:

<None Include="SessionRecords\Microsoft.Azure.Commands.ServiceBus.Test.ScenarioTests.ServiceBusDRConfigurationTests\ServiceBusDRConfigurationsCURDAlterName.json"> <CopyToOutputDirectory>Always</CopyToOutputDirectory> </None>

cormacpayne · 2018-07-23T23:10:20Z

src/ResourceManager/ServiceBus/Commands.ServiceBus.Test/ScenarioTests/ServiceBusTopicTests.ps1

-	# Delete the created Topic
-	$ResultDeleteTopic = Remove-AzureRmServiceBusTopic -ResourceGroupName $resourceGroupName -Namespace $namespaceName -Name $ResulListTopic[0].Name
-	Assert-True {$ResultDeleteTopic} "Topic not deleted"
+	## Delete the created Topic


@v-Ajnava please delete commented-out code (this applies everywhere in this PR)

cormacpayne · 2018-07-23T23:10:45Z

src/ResourceManager/ServiceBus/Commands.ServiceBus/ChangeLog.md

@@ -20,6 +20,8 @@
 -->
 ## Current Release
 * Updated help files to include full parameter types and correct input/output types.
+* Updated piping for InputObject and ResourceId in remove cmdlets
+* fixed few issues


@v-Ajnava would you mind adding a sub-bullet(s) that link to the issues fixed where applicable?

cormacpayne · 2018-07-23T23:15:41Z

src/ResourceManager/ServiceBus/Commands.ServiceBus/Cmdlets/AzureServiceBusCmdletBase.cs

+            switch (tokens.Length)
+            {
+                case 2:
+                    ParentResource = tokens[1];


@v-Ajnava what string is expected to be passed into this function? If it's a normal resource id (e.g., /subscription/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/xxxxxx/Microsoft.PROVIDER/PARENT1/xxxxxx/.....), then wouldn't tokens[1] be the subscription id?

Yes, its a resource id, but the tokens here will be the ParentResource property of ResourceIdentifier (Microsoft.Azure.Management.Internal.Resources.Utilities.Models) class. Tokens will have only parent resources.

cormacpayne · 2018-07-23T23:16:03Z

...ourceManager/ServiceBus/Commands.ServiceBus/Cmdlets/Namespace/NewAzureServiceBusNamespace.cs

+        /// <summary>
+        /// The Service Bus Premium namespace throughput units.
+        /// </summary>        
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, Position = 4, HelpMessage = "The Service Bus premium namespace throughput units, allowed values 1 or 2 or 4")]


@v-Ajnava please remove the Position property from this parameter

cormacpayne · 2018-07-23T23:19:55Z

...ger/ServiceBus/Commands.ServiceBus/Cmdlets/Subscription/RemoveAzureServiceBusSubscription.cs

            // delete a Subscription 
            if (ShouldProcess(target: Name, action: string.Format(Resources.RemoveSubscription, Name, Topic, Namespace)))
            {
-                WriteObject(Client.DeleteSubscription(ResourceGroupName, Namespace, Topic, Name));


@v-Ajnava what was this operation returning before? We should make sure we are still writing the same type to the output stream

cormacpayne · 2018-07-23T23:20:11Z

src/ResourceManager/ServiceBus/Commands.ServiceBus/Cmdlets/Topic/RemoveAzureServiceBusTopic.cs

 namespace Microsoft.Azure.Commands.ServiceBus.Commands.Topic
 {
    /// <summary>
    /// 'Remove-AzureRmServiceBusTopic' Cmdlet removes the specified ServiceBus Topic
    /// </summary>
-    [Cmdlet(VerbsCommon.Remove, ServicebusTopicVerb , SupportsShouldProcess = true)]
+    [Cmdlet(VerbsCommon.Remove, ServicebusTopicVerb, DefaultParameterSetName = TopicPropertiesParameterSet, SupportsShouldProcess = true)]


@v-Ajnava same comment about output type

cormacpayne · 2018-07-23T23:20:37Z

src/ResourceManager/ServiceBus/Commands.ServiceBus/Cmdlets/Topic/RemoveAzureServiceBusTopic.cs

            if (ShouldProcess(target: Name, action: string.Format(Resources.RemoveTopic, Name, Namespace)))
            {
-                WriteObject(Client.DeleteQueue(ResourceGroupName, Namespace, Name));


@v-Ajnava same comment about what was previously being written to the output stream

cormacpayne · 2018-07-23T23:23:31Z

src/ResourceManager/ServiceBus/Commands.ServiceBus/help/Remove-AzureRmServiceBusNamespace.md

@@ -29,13 +42,46 @@ PS C:\> Remove-AzureRmServiceBusNamespace -ResourceGroup Default-ServiceBus-West

 Removes the Service Bus namespace `SB-Example1` from the specified resource group `Default-ServiceBus-WestUS`.

+### Example 2 - InputObject
+```
+PS C:\> Remove-AzureRmServiceBusNamespace -InputObject $inputobject


@v-Ajnava for the examples where you are using -InputObject, would you mind adding a line showing the user where they get value from?

PS C;\> $InputObject = Get-AzureRmServiceBusNamespace <params> PS C:\> Remove-AzureRmServiceBusNamespace -InputObject $InputObject

or better yet, you could show the user the piping scenario we are trying to enable with this parameter set:

PS C:\> Get-AzureRmServiceBusNamespace <params> | Remove-AzureRmServiceBusNamespace

then you could provide a snippet below this example letting them know it binds the result of Get-AzureRmServiceBusNamespace to the -InputObject parameter of the Remove-AzureRmServiceBusNamespace cmdlet

cormacpayne · 2018-07-23T23:26:19Z

src/ResourceManager/ServiceBus/Commands.ServiceBus/help/Remove-AzureRmServiceBusNamespace.md

+
+### Example 3 - ResourceId
+```
+PS C:\> Remove-AzureRmServiceBusNamespace -ResourceId $resourceid


@v-Ajnava similar comment to the one above for -InputObject (and it also applies for the examples where you're using -ResourceId)
Using a separate variable:

PS C:\> $ResourceId = (Get-AzureRmResource -ResourceType Microsoft.ServiceBus/namespaces).ResourceId PS C:\> Remove-AzureRmServiceBusNamespace -ResourceId $ResourceId

Using piping:

PS C:\> Get-AzureRmResource -ResourceType Microsoft.ServiceBus/namespaces | Remove-AzureRmServiceBusNamespace

…nto preview_ipobject # Conflicts: # src/ResourceManager/ServiceBus/Commands.ServiceBus/ChangeLog.md # src/ResourceManager/ServiceBus/Commands.ServiceBus/Cmdlets/Namespace/RemoveAzureServiceBusNamespace.cs # src/ResourceManager/ServiceBus/Commands.ServiceBus/Cmdlets/Queue/RemoveAzureServiceBusQueue.cs # src/ResourceManager/ServiceBus/Commands.ServiceBus/Cmdlets/Topic/RemoveAzureServiceBusTopic.cs # src/ResourceManager/ServiceBus/Commands.ServiceBus/help/Remove-AzureRmServiceBusNamespace.md

cormacpayne · 2018-07-25T16:22:01Z

src/ResourceManager/ServiceBus/Commands.ServiceBus/Cmdlets/Queue/RemoveAzureServiceBusQueue.cs

+        public string ResourceId { get; set; }
+
+        [Parameter(Mandatory = false)]
+        public SwitchParameter PassThru { get; set; }


@v-Ajnava it doesn't look like the -PassThru parameter is being used anymore. I might have been a little unclear with the comment I made regarding this. I wanted to make sure that whatever was returned by the operation below was still being returned. You can still assign the result to a variable, and if the user provides -PassThru, then you write that object to the output stream.

var result = Client.DeleteQueue(ResourceGroupName, Namespace, Name); if (PassThru.IsPresent) { WriteObject(result); }

@v-Ajnava this comment applies to the other affected areas

v-Ajnava · 2018-07-25T23:56:09Z

@cormacpayne the test had failed, so re-recorded and pushed the recordings. the push dismissed your review. can you please review it?

v-ajnava added 2 commits July 23, 2018 10:21

piping scenarion for Remove cmdlets and fixes for issues

99c143b

updated

7d945d5

azuresdkci added the needs-review label Jul 23, 2018

v-ajnava added 3 commits July 23, 2018 15:42

updated change log

f1dedf2

updated PassThru message in markdown

ee6c64c

v-Ajnava changed the title ~~Service Bus: some issues fix and piping for remocnl~~ Service Bus: some issues fix and piping for remove cmdlet Jul 23, 2018

cormacpayne suggested changes Jul 23, 2018

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Jul 23, 2018

cormacpayne assigned v-Ajnava Jul 23, 2018

cormacpayne mentioned this pull request Jul 23, 2018

Event Hub: some issues fix and piping for remove cmdlet #6733

Merged

8 tasks

v-ajnava added 3 commits July 24, 2018 14:19

review comments

431e9f0

review comments updated and fixed signature issue

d4a03db

cormacpayne reviewed Jul 25, 2018

View reviewed changes

v-Ajnava assigned cormacpayne and unassigned v-Ajnava Jul 25, 2018

v-Ajnava added needs-review and removed needs-revision labels Jul 25, 2018

v-ajnava added 2 commits July 25, 2018 10:47

fixed PassThru

1f3432b

re-recorded test

915cae3

cormacpayne previously approved these changes Jul 25, 2018

View reviewed changes

cormacpayne changed the base branch from preview to release-2018-07-27 July 25, 2018 21:22

re-recorded failing tests

76306d6

v-Ajnava dismissed cormacpayne’s stale review via 76306d6 July 25, 2018 23:06

cormacpayne approved these changes Jul 26, 2018

View reviewed changes

cormacpayne merged commit 815187a into Azure:release-2018-07-27 Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service Bus: some issues fix and piping for remove cmdlet #6732

Service Bus: some issues fix and piping for remove cmdlet #6732

v-Ajnava commented Jul 23, 2018 •

edited

Loading

cormacpayne left a comment

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 24, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 23, 2018

v-Ajnava Jul 25, 2018

cormacpayne Jul 25, 2018

cormacpayne Jul 25, 2018

v-Ajnava commented Jul 25, 2018 •

edited

Loading

Service Bus: some issues fix and piping for remove cmdlet #6732

Service Bus: some issues fix and piping for remove cmdlet #6732

Conversation

v-Ajnava commented Jul 23, 2018 • edited Loading

Description

cormacpayne left a 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

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

v-Ajnava commented Jul 25, 2018 • edited Loading

v-Ajnava commented Jul 23, 2018 •

edited

Loading

v-Ajnava commented Jul 25, 2018 •

edited

Loading