ServiceBus CLI Extension by v-Ajnava · Pull Request #31 · Azure/azure-cli-extensions

v-Ajnava · 2018-01-09T17:43:18Z

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

Have you run ./scripts/ci/test_static.sh locally? (pip install pylint flake8 required)
Have you run python scripts/ci/test_integration.py -q locally?

…into SBmasterExt

derekbekoe · 2018-01-10T23:53:10Z

@v-Ajnava Is this ready for review?

v-Ajnava · 2018-01-10T23:57:44Z

@derekbekoe yes, you can review it.

derekbekoe · 2018-01-11T00:03:20Z

+
+helps['servicebus'] = """
+    type: group
+    short-summary: Manage Azure ServiceBus namespace, queue, topic, subscription, rule and geo disaster recovery configuration - alias


I believe the proper service name is 'Service Bus' not 'ServiceBus'.
Can you change this in the help file?

derekbekoe · 2018-01-11T00:14:57Z

+
+# pylint: disable=unused-import
+
+


Load the _helps file here otherwise it won't be loaded and so no help is shown.

e.g. https://github.com/Azure/azure-cli/blob/dev/src/command_modules/azure-cli-batchai/azure/cli/command_modules/batchai/__init__.py#L8

derekbekoe · 2018-01-11T00:15:50Z

+    type: command
+    short-summary: check for the availability of the given name for the Namespace
+    examples:
+        - name: Create a new topic.


The description title is wrong?
How does servicebus namespace check-name-availability create a new topic?

derekbekoe · 2018-01-11T00:16:09Z

+    short-summary: check for the availability of the given name for the Namespace
+    examples:
+        - name: Create a new topic.
+          text: az servicebus namespace check_name_availability --name mynamespace


check_name_availability -> check-name-availability

derekbekoe · 2018-01-11T00:16:34Z

+    examples:
+        - name: Create a new namespace.
+          text: helps['az servicebus namespace create --resource-group myresourcegroup --name mynamespace --location westus
+           --tags ['tag1: value1', 'tag2: value2'] --sku-name Standard --sku-tier Standard']


Bad help text.
Remove the 'helps' and make sure it's a command that can actually be executed.

Same below...

My Bad, i had observed it but missed to change it. its now resolved

derekbekoe · 2018-01-11T00:17:48Z

+
+helps['servicebus namespace authorizationrule list-keys'] = """
+    type: command
+    short-summary: Shows the connectionstrings of AuthorizatioRule for the namespace


connectionstrings should be two words not one.

derekbekoe · 2018-01-11T00:19:28Z

+        c.argument('correlation_id', options_list=['--correlation-id'], help='Identifier of the correlation.')
+        c.argument('message_id', options_list=['--message-id'], help='Identifier of the message.')
+        c.argument('to', options_list=['--to'], help='Address to send to.')
+        c.argument('reply_to', options_list=['--reply_to'], help='Address of the queue to reply to.')


--reply_to -> --reply-to

derekbekoe · 2018-01-11T00:30:22Z

+    author_email='v-ajnava@microsoft.com',
+    url='https://github.com/Azure/azure-cli-extensions',
+    classifiers=CLASSIFIERS,
+    packages=find_packages(),


Change to find_packages(exclude=["tests"])

Let me know if the CI tests check fails because of this.
Ideally we don't want tests in the built package.

derekbekoe · 2018-01-11T00:32:10Z

+    name='servicebus',
+    version=VERSION,
+    description='An Azure CLI Extension to manage servicebus resources',
+    long_description='An Azure CLI Extension to manage servicebus resources',


'Service Bus'

derekbekoe · 2018-01-11T00:43:21Z

+                 ' --alias {aliasname}')
+
+        getaliasafterbreak = self.cmd(
+            'servicebus georecovery-alias show  --resource-group {rg} --namespace-name {namespacenameprimary} --alias {aliasname}').output


You can use get_output_in_json() so no need to do the json.loads.

v-Ajnava · 2018-01-15T03:30:58Z

@derekbekoe , we have below cmdlet name changes

regenerate-keys to keys renew
check-name-availability to exists
servicebus georecovery-alias break-pairing to servicebus georecovery-alias break-pair

derekbekoe

@v-Ajnava I started reviewing but there are still multiple things wrong with this PR. This is what I have noted down so far.
Also, there are still some typos such as with --name in az servicebus namespace exists

derekbekoe · 2018-01-16T23:36:12Z

+    examples:
+        - name: Create a Service Bus Namespace.
+          text: az servicebus namespace create --resource-group myresourcegroup --name mynamespace --location westus
+           --tags ['tag1' 'value1', 'tag2' 'value2'] --sku-name Standard --sku-tier Standard


--tags tag1=value1 tag2=value2
What you currently have doesn't work.

derekbekoe · 2018-01-16T23:37:32Z

+    short-summary: Shows the Service Bus Namespace details
+    examples:
+        - name: shows the Namespace details.
+          text: helps['az servicebus namespace show --resource-group myresourcegroup --name mynamespace']


az servicebus namespace show --resource-group myresourcegroup --name mynamespace
What you currently have doesn't work.

derekbekoe · 2018-01-16T23:37:37Z

+    short-summary: List the Service Bus Namespaces by ResourceGroup or by subscription
+    examples:
+        - name: Get the Service Bus Namespaces by resource Group.
+          text: helps['az servicebus namespace list --resource-group myresourcegroup']


derekbekoe · 2018-01-16T23:37:49Z

+
+helps['servicebus namespace list'] = """
+    type: command
+    short-summary: List the Service Bus Namespaces by ResourceGroup or by subscription


Remove ' by ResourceGroup or by subscription'.
This follows the convention of other list commands.

derekbekoe · 2018-01-16T23:38:53Z

+    type: command
+    short-summary: List the Service Bus Namespaces by ResourceGroup or by subscription
+    examples:
+        - name: Get the Service Bus Namespaces by resource Group.


nit: lower case G for Group

derekbekoe · 2018-01-16T23:39:37Z

+    examples:
+        - name: Creates Authorizationrule 'myauthorule' for the given Service Bus Namespace 'mynamepsace' in resourcegroup
+          text: az servicebus namespace authorizationrule create --resource-group myresourcegroup --namespace-name mynamespace
+           --name myauthorule --access-rights [Send, Listen]


This won't work. If you want both Send and Listen it should be --access-rights Send Listen I guess...

derekbekoe · 2018-01-16T23:40:16Z

+    examples:
+        - name: Creates Service Bus Queue.
+          text: az sb queue create --resource-group myresourcegroup --namespace-name mynamespace --name myqueue
+"""


There's no command az sb queue create.
Should this be az servicebus queue create?

derekbekoe · 2018-01-16T23:41:19Z

+    examples:
+        - name: Creates Authorization rules for Queue
+          text: az servicebus queue authorizationrule create --resource-group myresourcegroup --namespace-name mynamespace
+           --queue-name myqueue --name myauthorule --access-rights [Listen]


Listen not [Listen]

derekbekoe · 2018-01-16T23:41:55Z

+    short-summary: Creates the Service Bus Topic
+    examples:
+        - name: Create a new queue.
+          text: az servicebus topic create --resource-group myresourcegroup --namespace-name mynamespace --name {topicname}


Remove {} around {topicname}

derekbekoe · 2018-01-16T23:42:15Z

+    short-summary: Shows the Service Bus Topic Details
+    examples:
+        - name: Shows the Topic details.
+          text: az sb topic get --resource-group myresourcegroup --namespace-name mynamespace --name {topicname}


Same here.
And there is no az sb topic show command.

v-Ajnava · 2018-01-17T19:55:38Z

@derekbekoe, have fixed the help issues.

…into SBmasterExt

derekbekoe

If the change to _params.py is too much for you to change, you can leave as is. Just note that arguments_context doesn't have to apply to a specific command. It can apply to a whole group such as all commands under servicebus. This would have made the params.py a lot smaller as a lot of the params are just duplicated.

derekbekoe · 2018-01-18T18:18:15Z

+
+helps['servicebus queue authorizationrule keys list'] = """
+    type: command
+    short-summary: Shows the connectionstrings of AuthorizationRule for the Queue.


Shouldn't 'connectionstrings' be two words?

derekbekoe · 2018-01-18T18:21:03Z

+# pylint: disable=line-too-long
+def load_arguments_namespace(self, _):
+    with self.argument_context('servicebus namespace exists') as c:
+        c.argument('namespace_name', options_list=['--name'], help='name of the Namespace')


For all --name options, also add -n to the list so short options work. They are very common in the CLI.

derekbekoe · 2018-01-18T18:24:00Z

+            c.argument('namespace_name', options_list=['--name', '-n'], help='name of the Namespace')
+
+    with self.argument_context('servicebus namespace list') as c:
+        c.argument('resource_group_name', arg_type=resource_group_name_type)


Do this at the start of _params.py.

Then you don't have to keep on doing it for each command as it will apply for all.
Then you can remove all the c.argument('resource_group_name', arg_type=resource_group_name_type,) everywhere.

with self.argument_context('servicebus') as c: c.argument('resource_group_name', arg_type=resource_group_name_type)

Resolved, please verify .

derekbekoe · 2018-01-18T18:30:51Z

+           --name myauthorule
+"""
+
+helps['sb queue create'] = """


This is still wrong here...

derekbekoe · 2018-01-18T18:31:00Z

+          text: az servicebus topic create --resource-group myresourcegroup --namespace-name mynamespace --name mytopic
+"""
+
+helps['sb topic show'] = """


Same here..

derekbekoe · 2018-01-18T18:31:08Z

+           --subscription-name mysubscription --name myrule
+"""
+
+helps['sb rule list'] = """


derekbekoe · 2018-01-18T21:48:36Z

Looks good.
Please add the metadata file.
Like this:
https://github.com/Azure/azure-cli-extensions/blob/master/src/image-copy/azext_imagecopy/azext_metadata.json

You can set the min version to 2.0.25.

v-Ajnava · 2018-01-18T22:39:14Z

@derekbekoe I added the metadata file.

Ajit Maruti Navasare (MINDTREE LIMITED) added 7 commits January 9, 2018 09:41

ServiceBus CLI Extension

3d184b0

updated Codeowners

79e2369

changed sb to servicebus

41be029

fixed lint error

270f3e1

Merge branch 'master' of https://github.com/Azure/azure-cli-extensions …

4c9ba91

…into SBmasterExt

updated help

247a580

Merge branch 'master' of https://github.com/Azure/azure-cli-extensions …

ca2ebf4

…into SBmasterExt

derekbekoe suggested changes Jan 11, 2018

View reviewed changes

Ajit Maruti Navasare (MINDTREE LIMITED) added 5 commits January 11, 2018 14:02

Review comments resolved

3b0dac4

Review comments resolved

2291a74

fixed the lint error

68e2c0d

lint error of trailing whitespace fixed

b55b9cf

test fixture

4203d5c

derekbekoe suggested changes Jan 16, 2018

View reviewed changes

resolved help review comments

0b686be

Merge branch 'master' of https://github.com/Azure/azure-cli-extensions …

b8c2575

…into SBmasterExt

derekbekoe suggested changes Jan 18, 2018

View reviewed changes

Review comments for help and params

c65326d

Added Matadat file

3c5bac2

derekbekoe approved these changes Jan 18, 2018

View reviewed changes

derekbekoe merged commit 5e43e0e into Azure:master Jan 18, 2018

Conversation

v-Ajnava commented Jan 9, 2018

General Guidelines

Uh oh!

derekbekoe commented Jan 10, 2018

Uh oh!

v-Ajnava commented Jan 10, 2018

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!

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!

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!

v-Ajnava commented Jan 15, 2018

Uh oh!

derekbekoe left a comment • 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!

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!

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!

derekbekoe left a comment •

edited

Loading