Initial commit for Appliance commands #2985

vivsriaus · 2017-04-25T17:02:39Z

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

General Guidelines

The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

Each command and parameter has a meaningful description.
Each new command has a test.

msftclas · 2017-04-25T17:02:45Z

@vivsriaus,
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

troydai

Please fix code style and add tests to offer basic code coverage.

troydai · 2017-04-26T23:13:39Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        # split at the first ':', both principalId and roldeDefinitionId should not have a ':'
+        principalId, roleDefinitionId = name_value.split(':', 1)
+        #print('>>>>>>>principalId: {}'.format(principalId))
+        #print('>>>>>>>roleDefinitionId: {}'.format(roleDefinitionId))


Please remove the commented code.

troydai · 2017-04-26T23:13:47Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        applianceAuth.append(applianceAuth1)
+    #applianceAuth1 = ApplianceProviderAuthorization("d5d74787-be93-4a54-baae-e9ed29bfe353","8e3af657-a8ff-443c-a75c-2fe8c4bcb635")
+    #applianceAuth = [applianceAuth1]
+    #applianceDefProperties = ApplianceDefinitionProperties(ApplianceLockLevel.can_not_delete, applianceAuth, package_file_uri, display_name, None, description)


Remove commented code.

tjprescott · 2017-04-27T20:14:23Z

Please include the -h output for the group and the create/update commands. This aids in review.

…ppliance2

troydai · 2017-05-01T17:02:01Z

@vivsriaus please rebase your changes again. There are changes made to master branch results in the merge conflicts. Please make sure to keep your branch up-to-date so we can review and approve the changes timely. Otherwise, the fast evolving master branch may keep causing the merge conflicts. Please also be noted that we're running PEP8 check on most modules now, including the resource module. As a result, it is recommended to run check_style --ci before push to ensure code styles are compliant.

gandhiniraj · 2017-05-01T20:17:50Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

@@ -64,6 +73,110 @@ def create_resource_group(rg_name, location, tags=None):
    )
    return rcf.resource_groups.create_or_update(rg_name, parameters)

+def create_appliance(resource_group_name, appliance_name, managed_rg_id, location, kind, appliance_definition_id=None, plan_name=None, plan_publisher=None, plan_product=None, plan_version=None, tags=None, parameters=None):


managedby_resource_group_id

Sure. But it would be very verbose IMHO

gandhiniraj · 2017-05-01T20:23:07Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

@@ -64,6 +73,110 @@ def create_resource_group(rg_name, location, tags=None):
    )
    return rcf.resource_groups.create_or_update(rg_name, parameters)

+def create_appliance(resource_group_name, appliance_name, managed_rg_id, location, kind, appliance_definition_id=None, plan_name=None, plan_publisher=None, plan_product=None, plan_version=None, tags=None, parameters=None):


appliance - we made a decision call this "managed application" - can we change the cmdlet to create_managed_app

gandhiniraj · 2017-05-01T20:28:58Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+            raise CLIError('--appliance-definition-id is required if kind is ServiceCatalog')
+    elif kind.lower() == 'marketplace':
+        if plan_name is None and plan_product is None and plan_publisher is None and plan_version is None:
+            raise CLIError('--plan-name, --plan-product, --plan-publisher and --plan-version are all required if kind is ServiceCatalog')


this has to be marketplace

gandhiniraj · 2017-05-01T20:29:54Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        if plan_name is None and plan_product is None and plan_publisher is None and plan_version is None:
+            raise CLIError('--plan-name, --plan-product, --plan-publisher and --plan-version are all required if kind is ServiceCatalog')
+        else:
+            appliancePlan = Plan(plan_name, plan_publisher, plan_product, plan_version)


We don't need this intermediate variable

gandhiniraj · 2017-05-01T20:35:59Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+
+    return racf.appliances.create_or_update(resource_group_name, appliance_name, appliance)
+
+def show_appliance(resource_group_name=None, appliance_name=None, appliance_id=None):


Still learning this CLI, but shouldn't appliance_id be documented below?

It is in the params file. We don't necessarily add param description for all params in custom file

gandhiniraj · 2017-05-01T20:36:36Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        appliance = racf.appliances.get(resource_group_name, appliance_name)
+    return appliance
+
+def show_appliancedefinition(resource_group_name=None, appliance_definition_name=None, appliance_definition_id=None):


same documentation comment

Same as above.

gandhiniraj · 2017-05-01T20:38:19Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        appliancedef = racf.appliance_definitions.get(resource_group_name, appliance_definition_name)
+    return appliancedef
+
+def create_appliancedefinition(resource_group_name, appliance_definition_name, location, lock_level, package_file_uri, authorizations, description, display_name, tags=None):


just making sure o camel casing? Is that not standard if not please ignore

gandhiniraj · 2017-05-01T20:39:49Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+    '''
+    racf = _resource_appliances_client_factory()
+    authorizations = authorizations or []
+    applianceAuth = []


applianceAuthList so that you don't have to name applianceAuth1 below

Makes sense

gandhiniraj · 2017-05-01T20:40:45Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        principalId, roleDefinitionId = name_value.split(':', 1)
+        applianceAuth1 = ApplianceProviderAuthorization(principalId, roleDefinitionId)
+        applianceAuth.append(applianceAuth1)
+    applianceDefProperties = ApplianceDefinitionProperties(lock_level, applianceAuth, package_file_uri, display_name, None, description)


Why one of the param is "None"?

I've flattened the Appliance model. We no longer have properties model for both Appliance and ApplianceDefinitions

gandhiniraj · 2017-05-01T20:41:21Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        principalId, roleDefinitionId = name_value.split(':', 1)
+        applianceAuth1 = ApplianceProviderAuthorization(principalId, roleDefinitionId)
+        applianceAuth.append(applianceAuth1)
+    applianceDefProperties = ApplianceDefinitionProperties(lock_level, applianceAuth, package_file_uri, display_name, None, description)


nit: applianceDefinitionProperties

NA anymore after flattening

gandhiniraj

Please see my suggestions/comments

…ppliance2 # Conflicts: # src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

…ppliance2

vivsriaus

Addressed comments

vivsriaus · 2017-05-02T20:27:15Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

@@ -64,6 +73,110 @@ def create_resource_group(rg_name, location, tags=None):
    )
    return rcf.resource_groups.create_or_update(rg_name, parameters)

+def create_appliance(resource_group_name, appliance_name, managed_rg_id, location, kind, appliance_definition_id=None, plan_name=None, plan_publisher=None, plan_product=None, plan_version=None, tags=None, parameters=None):


vivsriaus · 2017-05-02T20:34:43Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

@@ -64,6 +73,110 @@ def create_resource_group(rg_name, location, tags=None):
    )
    return rcf.resource_groups.create_or_update(rg_name, parameters)

+def create_appliance(resource_group_name, appliance_name, managed_rg_id, location, kind, appliance_definition_id=None, plan_name=None, plan_publisher=None, plan_product=None, plan_version=None, tags=None, parameters=None):


Sure. But it would be very verbose IMHO

vivsriaus · 2017-05-02T20:35:47Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        if plan_name is None and plan_product is None and plan_publisher is None and plan_version is None:
+            raise CLIError('--plan-name, --plan-product, --plan-publisher and --plan-version are all required if kind is ServiceCatalog')
+        else:
+            appliancePlan = Plan(plan_name, plan_publisher, plan_product, plan_version)


vivsriaus · 2017-05-02T20:36:45Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+
+    return racf.appliances.create_or_update(resource_group_name, appliance_name, appliance)
+
+def show_appliance(resource_group_name=None, appliance_name=None, appliance_id=None):


It is in the params file. We don't necessarily add param description for all params in custom file

vivsriaus · 2017-05-02T20:36:57Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        appliance = racf.appliances.get(resource_group_name, appliance_name)
+    return appliance
+
+def show_appliancedefinition(resource_group_name=None, appliance_definition_name=None, appliance_definition_id=None):


Same as above.

vivsriaus · 2017-05-02T20:38:13Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        principalId, roleDefinitionId = name_value.split(':', 1)
+        applianceAuth1 = ApplianceProviderAuthorization(principalId, roleDefinitionId)
+        applianceAuth.append(applianceAuth1)
+    applianceDefProperties = ApplianceDefinitionProperties(lock_level, applianceAuth, package_file_uri, display_name, None, description)


I've flattened the Appliance model. We no longer have properties model for both Appliance and ApplianceDefinitions

vivsriaus · 2017-05-02T20:38:28Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        principalId, roleDefinitionId = name_value.split(':', 1)
+        applianceAuth1 = ApplianceProviderAuthorization(principalId, roleDefinitionId)
+        applianceAuth.append(applianceAuth1)
+    applianceDefProperties = ApplianceDefinitionProperties(lock_level, applianceAuth, package_file_uri, display_name, None, description)


NA anymore after flattening

vivsriaus · 2017-05-02T20:40:00Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+    '''
+    racf = _resource_appliances_client_factory()
+    authorizations = authorizations or []
+    applianceAuth = []


Makes sense

…ppliance2

troydai · 2017-05-03T18:56:14Z

@vivsriaus changes look good. Waiting for tests.

…ppliance2

lmazuel

Important fix

lmazuel · 2017-05-03T23:57:13Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+    racf = _resource_managedapps_client_factory()
+
+    if resource_group_name:
+        return racf.appliances.list_by_resource_group(resource_group_name)


CLI does not support iterator, change this to:

if resource_group_name: result = racf.appliances.list_by_resource_group(resource_group_name) else: result = racf.appliances.list_by_subscription() return list(result)

@johanste should be documented? Twice issues like this today.

@lmazuel returning a list above still won't work for us, because "value" will be missing.

…ppliance2

…ppliance2 # Conflicts: # src/command_modules/azure-cli-vm/setup.py

vivsriaus · 2017-05-04T05:35:32Z

@lmazuel Thanks for the suggestion, modified the command to always return a list.

…ppliance2

vivsriaus · 2017-05-04T20:24:02Z

@azuresdkci test this please

…ppliance2

troydai · 2017-05-05T01:05:52Z

Last thing. Can you add a HIstory file?

The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

vivsriaus · 2017-05-05T03:50:18Z

@troydai Modified HISTORY.rst

Initial commit for Appliance commands

a5765d4

msftclas added the cla-not-required label Apr 25, 2017

derekbekoe added the do-not-merge label Apr 25, 2017

derekbekoe changed the title ~~[Do Not Merge] Initial commit for Appliance commands~~ Initial commit for Appliance commands Apr 25, 2017

troydai suggested changes Apr 26, 2017

View reviewed changes

tjprescott added the New Command(s) label Apr 27, 2017

vivsriaus added 2 commits April 28, 2017 14:10

Update appliance commands based on feedback

e772625

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

3c46e73

…ppliance2

troydai added this to the Build Milestone milestone May 1, 2017

gandhiniraj reviewed May 1, 2017

View reviewed changes

gandhiniraj suggested changes May 1, 2017

View reviewed changes

vivsriaus added 4 commits May 1, 2017 14:27

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

a0f0477

…ppliance2 # Conflicts: # src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

Fix issue with missing auth mgmnt client

10d35cc

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

4bb8e7f

…ppliance2

Refactor after SDK update, address comments

3d749bd

vivsriaus commented May 2, 2017

View reviewed changes

vivsriaus added 2 commits May 2, 2017 13:50

Merge from upstream master

7c3de86

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

b69bd2a

…ppliance2

vivsriaus added 5 commits May 3, 2017 12:09

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

e950005

…ppliance2

Add delete managedapp definition test

e200cdc

Import JMESCheck as JCheck, to avoid conflict with existing tests

608ee11

Fix cf for app def

f326533

Add managedapp tests

49f3419

lmazuel suggested changes May 3, 2017

View reviewed changes

vivsriaus added 3 commits May 3, 2017 17:34

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

9b1ca03

…ppliance2

return list()

94d2346

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

b17b098

…ppliance2 # Conflicts: # src/command_modules/azure-cli-vm/setup.py

lmazuel approved these changes May 4, 2017

View reviewed changes

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

46f7a62

…ppliance2

vivsriaus added 6 commits May 4, 2017 13:32

Update help

7f109fe

Update help again

0aaa0e4

Update to ARM rc3 package, update tests

46e0e25

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

a965420

…ppliance2

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

8f6bd5b

…ppliance2

Add test for delete managedapp

6275150

troydai approved these changes May 5, 2017

View reviewed changes

Update HISTORY.rst

7069d2d

troydai merged commit 38e2936 into Azure:master May 5, 2017

haroldrandom added cla-not-required do-not-merge New Command(s) P1 labels Oct 25, 2019


		return racf.appliances.create_or_update(resource_group_name, appliance_name, appliance)

		def show_appliance(resource_group_name=None, appliance_name=None, appliance_id=None):

Initial commit for Appliance commands #2985

Initial commit for Appliance commands #2985

Conversation

vivsriaus commented Apr 25, 2017

General Guidelines

Command Guidelines

msftclas commented Apr 25, 2017

troydai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott commented Apr 27, 2017

troydai commented May 1, 2017

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

gandhiniraj May 1, 2017 • edited

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

gandhiniraj left a comment

Choose a reason for hiding this comment

vivsriaus 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

troydai commented May 3, 2017

lmazuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivsriaus commented May 4, 2017

vivsriaus commented May 4, 2017

troydai commented May 5, 2017

vivsriaus commented May 5, 2017

gandhiniraj May 1, 2017 •

edited