[AMS CLI] Added Azure Media Service CLI Module #6044

lucasmarambio · 2018-04-06T22:31:03Z

First version of Azure Media Services CLI Module. This version supports the following commands:

Account commands

az ams account create
az ams account update
az ams account show
az ams account list
az ams account delete
az ams account storage add
az ams account storage remove
az ams account sp create
az ams account sp reset-credentials

Transform commands

az ams transform create
az ams transform show
az ams transform list
az ams transform delete
az ams transform update
az ams transform output add
az ams transform output remove

Asset commands

az ams asset create
az ams asset update
az ams asset show
az ams asset list
az ams asset delete
az ams asset get-sas-urls

Job commands

az ams job start
az ams job show
az ams job list
az ams job cancel
az ams job delete

Streaming commands

az ams streaming locator create
az ams streaming locator show
az ams streaming locator list
az ams streaming locator get-paths
az ams streaming locator delete
az ams streaming policy create
az ams streaming policy show
az ams streaming policy list
az ams streaming policy delete

promptws · 2018-04-06T22:31:05Z

View a preview at https://prompt.ws/r/Azure/azure-cli/6044
This is an experimental preview for @microsoft users.

lucasmarambio · 2018-04-09T21:36:27Z

@tjprescott the pull request is ready for review

derekbekoe · 2018-04-09T23:10:38Z

Why is the SDK in azure/mediav3?

damatojuan · 2018-04-10T14:16:17Z

@derekbekoe we included the media SDK because it's not available in https://github.com/Azure/azure-sdk-for-python yet since is still in preview. We autogenerated it from https://github.com/Azure/azure-rest-api-specs/tree/MediaServices-2018-03-30-preview/specification/mediaservices/resource-manager/Microsoft.Media/preview/2018-03-30-preview.
If your question is about why we used the azure/mediav3 namespace, that's because we didn't find any guideline or documentation about placing an SDK inside the Azure-CLI so we based on https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/commands/client_factory.py#L32.

tjprescott

There are some coding issues I identified. Reach out to me if any don't make sense. Overall, looks good and the test coverage looks great. Would like @sptramer to review the help and @yugangw-msft to look over the SP create stuff.

tjprescott · 2018-04-10T16:50:10Z

src/command_modules/azure-cli-ams/HISTORY.rst

+Release History
+===============
+
+0.0.1


I believe this should be 0.1.0. @derekbekoe is that right? I realize we have done this differently in the past.

tjprescott · 2018-04-10T16:51:36Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/__init__.py

+class MediaServicesCommandsLoader(AzCommandsLoader):
+
+    def __init__(self, cli_ctx=None):
+        super(MediaServicesCommandsLoader, self).__init__(cli_ctx=cli_ctx)


You need to add the following kwarg:

super(MediaServicesCommandsLoader, self).__init__(cli_ctx=cli_ctx, min_profile='2017-03-10-profile')

Otherwise the module will appear on the Azure stack profile and cause errors.

tjprescott · 2018-04-10T16:55:46Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/_client_factory.py

+    client = GraphRbacManagementClient(cred, tenant_id,
+                                       base_url=cli_ctx.cloud.endpoints.active_directory_graph_resource_id)
+    configure_common_settings(cli_ctx, client)
+    return client


These are duplicated from the azure-cli-role module. What do you need these for? If other modules need them, we can consider moving them into the core.

This is used by the service principals commands. We need those methods (_auth_client_factory, _graph_client_factory) to retrieve the clients to manage the service principals for media accounts (create applications, grant access permissions, create secrets, etc).

Moving those methods to the core might be a good option. Is this something that you would like us to do? In that case, which will be the best place to put them? azure.cli.core.commands.client_factory seems a good place.

It would be nice to have them in the core for others to use. We really need to simplify the process of getting customer's their AAD application, Service Principals, and Keys for services that are moving their customers to AAD and off of ACS.

We are really complicating things when moving our customers off of simple Keys and trying to explain why they need an AAD AP, then a Service Principal, and then get a Key from that... very confusing stuff for our general customers who may not be core Azure devs.

This is also related to #6044 (comment)

tjprescott · 2018-04-10T16:58:16Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/_help.py

+helps['ams streaming policy show'] = """
+    type: command
+    short-summary: Show the details of a streaming policy.
+"""


@sptramer for help review.

tjprescott · 2018-04-10T16:59:50Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/_help.py

+    parameters:
+        - name: --account-name
+          short-summary: The name of the Azure Media Services account within the resource group.
+        - name: --name


I don't think this will be applied if it is not listed as:

- name: --name -n short-summary: blah blah

Is there a reason you have the parameter help in the help.py file for this one command?

You're right, we will list the help as you specified. We specified the parameters just because the --years argument doesn't show a fallback help text, so we'll remove the rest ones.

tjprescott · 2018-04-10T17:20:01Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/_params.py

+                   help='Enable Dash protocol.')
+        c.argument('hls',
+                   arg_group='Encryption Protocols',
+                   help='Enable Hls protocol.')


tjprescott · 2018-04-10T17:23:11Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/_validators.py

+        if not acc:
+            raise ValueError("Storage account named '{}' not found in the resource group '{}'.".
+                             format(namespace.storage_account, namespace.resource_group_name))
+        namespace.storage_account = acc.id  # pylint: disable=no-member


See this logic for the recommended way of implementing "name or ID" behavior:
https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#supporting-name-or-id-parameters

If it is NOT a valid ID (which there is a standard call for) then build the ID (in this case, assuming regular storageAccount not classic storage). Always assume it exists and let ARM fail if it doesn't. Don't do a GET just to make sure the account exists. If it does exists, the check wasn't necessary and you made extra REST calls for no reason. If it doesn't exist, it will fail either way.

tjprescott · 2018-04-10T17:24:01Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/commands.py

+            operations_tmpl='azure.cli.command_modules.ams.operations.{}#'.format(custom_module) + '{}',
+            client_factory=client_factory,
+            exception_handler=build_exception_wrapper()
+        )


The command registration seems much more verbose than I usually see, and I think the culprit may be this pattern. Did you emulate this pattern from an existing module?

Yes, we emulate this pattern from the storage module:

azure-cli/src/command_modules/azure-cli-storage/azure/cli/command_modules/storage/commands.py

Line 32 in 105d22c

def get_custom_sdk(custom_module, client_factory, resource_type=ResourceType.DATA_STORAGE):

Should we consider changing this implementation?

You really should not follow storage. Nearly any other module would be much simpler. Please look at network and see if that pattern is simpler. It should be.

tjprescott · 2018-04-10T17:27:28Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/tests/latest/test_ams.py

+    @ResourceGroupPreparer()
+    @StorageAccountPreparer(parameter_name='storage_account_for_create')
+    def test_ams_create_show(self, resource_group, storage_account_for_create):
+        amsname = self.create_random_name(prefix='ams', length=12)


Unless the AMS account name needs to be globally unique (like KeyVaults or StorageAccounts) you can get away with a static name here.

Given that unit tests run in parallel, using the same static account name for all tests leads to name collision. Media account names should be unique by location and all test accounts are created under the same location. We could use different static account names for each test, but we believe that generating names randomly minimizes the chance of potential collisions.

Fair enough. Most resources only need to be unique within the RG.

tjprescott · 2018-04-10T17:32:06Z

src/command_modules/azure-cli-ams/azure/cli/command_modules/ams/_exception_handler.py

+# --------------------------------------------------------------------------------------------
+
+
+def build_exception_wrapper(message=None):


When I look at where this is used in commands.py, it doesn't look like there's any need for this to be a factory. You can simply remove this and pass "build_exception" directly

exception_handler=build_exception

I would also rename build_exception to something more descriptive like ams_exception_handler.

Good catch, we'll clean and rename this exception handler.

tjprescott · 2018-04-10T17:35:01Z

@yugangw-msft would you please take a quick look at their sp create command using https://prompt.ws/r/Azure/azure-cli/6044 just to make sure it aligns with existing SP create commands?

yugangw-msft · 2018-04-10T17:54:09Z

 @yugangw-msft would you please take a quick look at their sp create command using https://prompt.ws/r/Azure/azure-cli/6044 just to make sure it aligns with existing SP create commands?

Guess it mostly copied and pasted the code from the azure-cli-role module. Any reason we can't point users to use the generic SP commands instead of mostly duplicating the same command here? The concerns is the SDK library of azure-mgmt-authorization (and azure-graphrbac possibly) will change, and the library upgrading could break the cloned commands.
Also it is common for cli users to use multiple command modules to get a workflow done. Only using commands from a single command doesn't necessary mean the superiority or better user experience, because having 2 places to do the same thing always ends up being confusing

lucasmarambio · 2018-04-12T22:15:09Z

Thanks for the feedback @yugangw-msft.

Adding @johndeu to help deciding the best approach. Perhaps the alternatives are:

Remove the SP commands from the azure-cli-ams and let users add permissions through the generic SP commands in azure-cli-role as @yugangw-msft suggests.
Extract the logic for SP management that is common between the ams and role modules to the core module. This might not be a straight forward tasks and might require a considerable effort.
Add an exception for this particular case to keep the duplicated pieces of code in both modules.

johndeu · 2018-04-12T22:46:36Z

Can you demonstrate the number of CLI commands required to do the equivalent amount of work with both workflows? I believe we were wrapping several steps into one in our command and returning the content in a format that was easier for our customers to consume. I'm trying to keep the AAD portion of our CLI as simple as possible, since we are targeting a slightly different audience than typical core Azure with some of our customer base. We will get a lot of customers that are Media focused, and just want to get a darn Key to connect to the API as easy as we can make it without learning about what AAD applications and a Service Principal really are.

lucasmarambio · 2018-04-13T14:33:39Z

We just had an offline conversation with @johndeu to review the options regarding the SP command in the AMS module and agreed that it would be important to keep the current implementation as-is for the first release. For media focused customers this SP command is not just an easy way to create service principals and assign roles, but also to consolidate all the necessary information to connect to an AMS account (which is provided in the form of JSON/XML format in the command output).

@yugangw-msft @tjprescott could we add an exception and proceed with the current approach for the first release of the module?

yugangw-msft · 2018-04-13T16:11:02Z

  @yugangw-msft @tjprescott could we add an exception and proceed with the current approach for the first release of the module?

I understand your situation. Could you please share with me the commands to accomplish the end to end flow? If you can do all to avoid people ever getting to the generic RBAC commands, then it is your call. But by looking at the command signatures you provided, I don't see it is complete at a basic level. Say, I don't see SP list/show/delete command, so wonder a bit for how you can help people find the SP they are interested? Also for generic RBAC commands, we will keep on improving it, particularly for "my sp" support, so you don't need to wait for 20 minutes to list 1000000+ ones your org has, and also we will make it work with MS-Graph someday. With your cloned effort, your service will be left out.

If you still decide to proceed, please write an end to end test so to avoid the breaking when CLI upgrades the SDK dependencies

yugangw-msft · 2018-04-13T16:27:54Z

>we included the media SDK because it's not available in https://github.com/Azure/azure-sdk-for-python yet since is still in preview.

@damatojuan, please create a wheel and check-in the binary to the privates folder which CI system recognizes. This way we can get rid of the generated SDK files from this PR and focus on the command logics, also the github web pages will be more responsive with less files involved

lucasmarambio · 2018-04-13T18:27:26Z

@yugangw-msft find some answers below:

I understand your situation. Could you please share with me the commands to accomplish the end to end flow?

With this command users create a media account:

az ams account create -n {amsname} -g {rg} --storage-account {storageAccount} -l {location}

With this command users create a service principal with {role} access to the media account:

az ams account sp create -a {amsname} -n {spName} -g {rg} -p {spPassword} --role {role}

The output of this command provides all the information required to connect to the media account using Azure AD authentication:

{
  "AadClientId": "1598b742-a872-421e-a874-2475037db34b",
  "AadEndpoint": "https://login.microsoftonline.com",
  "AadSecret": {spPassword},
  "AadTenantId": "479472dc-cfef-474c-ba12-cba2854af0f6",
  "AccountName": {amsname},
  "ArmAadAudience": "https://management.core.windows.net/",
  "ArmEndpoint": "https://management.azure.com/",
  "Region": {rg-location},
  "ResourceGroup": {rg},
  "SubscriptionId": "d9b02582-2f4f-4b5d-a3dc-ba0015515f7b"
}

Say, I don't see SP list/show/delete command, so wonder a bit for how you can help people find the SP they are interested?

We only intended to provide a way to easily create and associate a SP with a media account, that's why only the ams account sp create and ams account sp reset-credentials commands are supported. For additional management, users should use the generic SP commands from the role module.

If you still decide to proceed, please write an end to end test so to avoid the breaking when CLI upgrades the SDK dependencies

We are currently providing tests for each command which should fail if there is any breaking change in the SDK dependencies.

lucasmarambio · 2018-04-13T19:37:07Z

@damatojuan, please create a wheel and check-in the binary to the privates folder which CI system recognizes. This way we can get rid of the generated SDK files from this PR and focus on the command logics, also the github web pages will be more responsive with less files involved

@yugangw-msft the current version of the AMS SDK is still in preview, and some tweaks were required in the generated SDK files that need to be tracked here for the moment. Once AMS is released as part of the Azure Python SDK, we'll remove the generated SDK files from the repo.

yugangw-msft · 2018-04-13T19:41:56Z

@lucasmarambio I understand it is under preview. My point was, since we are not reviewing generated code, I suggest you check in the wheel (if you have Python SDK PR submitted, it should expose the link to the wheel you can download or you can build out on your own). This has the benefit that you can get CI green on you PR, also your setup file can stay unchanged; and all you need to do in future is to remove the privates\your-wheel-file once @lmazuel publishes the sdk for you.
EDIT, or your team will release the CLI w/o public SDKs? Please clarify

lucasmarambio · 2018-04-13T20:17:06Z

@lucasmarambio I understand it is under preview. My point was, since we are not reviewing generated code, I suggest you check in the wheel (if you have Python SDK PR submitted, it should expose the link to the wheel you can download or you can build out on your own). This has the benefit that you can get CI green on you PR, also your setup file can stay unchanged; and all you need to do in future is to remove the privates\your-wheel-file once @lmazuel publishes the sdk for you.

Looping @tjprescott as we agreed with him to place the generated SDK files in this PR. @tjprescott could you help us with this? I am not quite familiar with Python wheels so maybe I'm missing something here. The main concern I see is that we made changes to the generated SDK files for the AMS CLI to work properly while in preview, so we need to track those changes here for everything to be on sync.

yugangw-msft · 2018-04-13T20:23:05Z

To generate a wheel, you just run one command under the SDK folder with the setup.py: python setup.py bdist_wheel, and you should see the wheel generated under a sub-folder. It is you and @tjprescott's call by the way since you 2 will work the most on this PR. My main question was whether the CLI will be released w/o public Python SDK.

- Add MediaServicesCommandLoader - Remove unnecesary parameters help commands - Don't use tuple syntax - Register account_name in ams scope - Remove 'within the resource group' - Set account_name help text generic - Apply account_name_arg_type - Typo: built-in - Remove Customer-supplied - Remove exception factory

- Rename `storage_account_id` to `validate_storage_account_id` - Assume storage account id exists to build the storage_account_id. - Set flag for `ams job cancel` - Set flags for protocols in `ams streaming policy` - Support multiple permissions for `ams asset get-sas-urls`

…y_name` - Add more datetime validation

…f-7bc850cdd18729c690d8be31d245e93b

derekbekoe added this to the Sprint 36 (Build) milestone Apr 9, 2018

tjprescott added the New Service label Apr 10, 2018

tjprescott suggested changes Apr 10, 2018

View reviewed changes

tjprescott self-assigned this Apr 10, 2018

tjprescott requested review from sptramer and yugangw-msft April 10, 2018 17:33

lucasmarambio force-pushed the add/ams-module branch from 6e0ce04 to 0054220 Compare April 10, 2018 22:21

sptramer approved these changes Apr 11, 2018

View reviewed changes

lucasmarambio force-pushed the add/ams-module branch from 0054220 to d8d46bc Compare April 12, 2018 21:33

lucasmarambio force-pushed the add/ams-module branch from 1ef77bd to ad4d8a7 Compare April 13, 2018 15:38

lucasmarambio force-pushed the add/ams-module branch from e1dd406 to fc78003 Compare April 13, 2018 20:22

lucasmarambio force-pushed the add/ams-module branch from f7ad180 to 418e1ea Compare April 16, 2018 15:38

derekbekoe approved these changes Apr 16, 2018

View reviewed changes

tjprescott approved these changes Apr 17, 2018

View reviewed changes

Lucas Marambio and others added 25 commits April 24, 2018 19:37

Remove streaming locator and policy update help commands

798ec84

Add streaming endpoint list/start/stop implementation

05dbc19

Add streaming endpoint test

237244a

Fix pylint warnings

3044d1a

Change release version

21fb750

Add validation to expiry time

96da44c

- Set --content-policy-name succinct for `default_content_key_polic…

63a9d4e

…y_name` - Add more datetime validation

Fix pep8 warnings

5f0c2f7

Remove ad app for sp tests

e36ffd3

Remove 'parameters' from help

792833a

Update ams setup version

ae769b4

Revert number version upgrade

fc49cd8

Upgrade version to 0.1.0

760bdeb

Add missing helps

5ba3867

Update Swagger Files to commit Azure/azure-rest-api-specs@48f6365#dif…

b9b2717

…f-7bc850cdd18729c690d8be31d245e93b

Fix swagger files pep8 warnings

931c327

Update help message for streaming endpoint start/stop commands

2594639

Update references to target the latest SDK for Python version

47f291c

Update VS projects to include ams module

f9f0fc2

Update get-sas-urls to support only one permission

0195a32

Update azure-mgmt-media dependency to target 1.0.0rc1

78814b6

Add video and audio analyzer presets

0882788

Instance Audio analyzer preset

c777453

lucasmarambio force-pushed the add/ams-module branch from 89af7d0 to c777453 Compare April 24, 2018 22:37

Fix line too long on transform

a22ddef

tjprescott merged commit 0cf5f8b into Azure:dev Apr 25, 2018

damatojuan deleted the add/ams-module branch July 2, 2018 20:37

haroldrandom added the New Service label Oct 25, 2019

		# --------------------------------------------------------------------------------------------


		def build_exception_wrapper(message=None):

[AMS CLI] Added Azure Media Service CLI Module #6044

[AMS CLI] Added Azure Media Service CLI Module #6044

Conversation

lucasmarambio commented Apr 6, 2018 • edited Loading

Account commands

Transform commands

Asset commands

Job commands

Streaming commands

promptws commented Apr 6, 2018

lucasmarambio commented Apr 9, 2018

derekbekoe commented Apr 9, 2018

damatojuan commented Apr 10, 2018

tjprescott 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

johndeu Apr 12, 2018 • 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

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

tjprescott commented Apr 10, 2018

yugangw-msft commented Apr 10, 2018 • edited Loading

lucasmarambio commented Apr 12, 2018

johndeu commented Apr 12, 2018

lucasmarambio commented Apr 13, 2018

yugangw-msft commented Apr 13, 2018 • edited Loading

yugangw-msft commented Apr 13, 2018

lucasmarambio commented Apr 13, 2018

lucasmarambio commented Apr 13, 2018

yugangw-msft commented Apr 13, 2018 • edited Loading

lucasmarambio commented Apr 13, 2018

yugangw-msft commented Apr 13, 2018

lucasmarambio commented Apr 6, 2018 •

edited

Loading

johndeu Apr 12, 2018 •

edited

Loading

yugangw-msft commented Apr 10, 2018 •

edited

Loading

yugangw-msft commented Apr 13, 2018 •

edited

Loading

yugangw-msft commented Apr 13, 2018 •

edited

Loading