Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DocumentDB Azure CLI support #1815

Merged
merged 16 commits into from Feb 1, 2017

Conversation

Projects
None yet
7 participants
@dmakwana
Copy link
Contributor

commented Jan 21, 2017

This pull request contains the auto-generated python SDK, which will get removed before it gets merged in. This will require the imports to change to point to where the python SDK actually lives. Update command to come.

@azurecla

This comment has been minimized.

Copy link

commented Jan 21, 2017

Hi @dmakwana, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!


It looks like you're working at Microsoft (dimakwan). If you're full-time, we DON'T require a contribution license agreement.



If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

register_cli_argument('documentdb regenerate-key', 'key_kind', **enum_choice_list(KeyKind))
register_cli_argument('documentdb failover-priority-change', 'failover_policies', validator=validate_failover_policies, help="space separated failover policies in 'regionName=failoverPriority' format. E.g \"East US\"=0", nargs='+')

register_cli_argument('documentdb create', 'resource_group_location', help="location of the resource group")

This comment has been minimized.

Copy link
@lostintangent

lostintangent Jan 21, 2017

Contributor

Will there be a way to specify that you want a MongoDB API?

This comment has been minimized.

Copy link
@dmakwana

dmakwana Jan 27, 2017

Author Contributor

The swagger file needs to be updated to accept the Kind property in the DatabaseAccountCreateUpdateParameters. Once this happens, I'll add it as a parameter to 'documentdb create'

@joshgav
Copy link

left a comment

Perhaps you listed "resource_group_location" twice and left out "resource_group_name"?

from azure.cli.command_modules.documentdb.sdk.models.document_db_enums import DefaultConsistencyLevel
from azure.cli.command_modules.documentdb.sdk.models.failover_policy import FailoverPolicy
from azure.cli.command_modules.documentdb.sdk.models.location import Location
from azure.cli.command_modules.documentdb.sdk.models.location import Location

This comment has been minimized.

Copy link
@joshgav

joshgav Jan 23, 2017

repeated line

register_cli_argument('documentdb create', 'resource_group_location', help="location of the resource group")
register_cli_argument('documentdb create', 'locations', validator=validate_locations, help="space separated locations in 'regionName=failoverPriority' format. E.g \"East US\"=0", nargs='+')
register_cli_argument('documentdb create', 'default_consistency_level', help="default consistency level", **enum_choice_list(DefaultConsistencyLevel))
register_cli_argument('documentdb create', 'resource_group_location', help="location of the resource group")

This comment has been minimized.

Copy link
@joshgav

joshgav Jan 23, 2017

repeat of 44

@joshgav

This comment has been minimized.

Copy link

commented Jan 23, 2017

FWIW would be easier to type if the group was named "docdb" instead of "documentdb"

@tjprescott
Copy link
Member

left a comment

You need to address any Pylint errors for it to pass the CI. Also, please include a comment for the PR which shows the output of {command} -h for each of the non-trivial commands (create, regenerate-keys, etc. you can omit the global arguments section).

helps['documentdb'] = """
type: group
short-summary: Managed NoSQL Database
long-summary:

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

You can omit this if there is no long summary.

comps = item.split('=', 1)
fp_dict.append(FailoverPolicy(comps[0], int(comps[1])))
ns.failover_policies = fp_dict
return ns

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

You don't need to return the namespace from validators.

comps = item.split('=', 1)
loc_dict.append(Location(location_name=comps[0], failover_priority=int(comps[1])))
ns.locations = loc_dict
return ns

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

Same here. No need to return ns.

return ns

def validate_locations(ns):
''' Extracts multiple space-separated locations in regionName=failoverPriority format '''

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

What is the difference between validate_failover_policies and validate_locations?

This comment has been minimized.

Copy link
@dmakwana

dmakwana Jan 27, 2017

Author Contributor

The location class contains additional readonly variables such as documentEndpoint and provisioningState which don't exist in the failoverPolicy class. For the sake of consistency and clarity I used their respective classes even though the input format is the same

ns.locations = loc_dict
return ns

register_cli_argument('documentdb', 'account_name', arg_type=name_type, help='Name of the DocumentDB Database Account', completer=get_resource_name_completion_list('Microsoft.DocumentDb/databaseAccounts'), id_part="name")

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

You will also need an entry like so:
register_cli_argument('documentdb create', 'account_name', completer=None)

Otherwise you will have tab completion for account name when creating an account, which doesn't make sense since you want to create an account that doesn't exist.

import azure.cli.command_modules.documentdb
from azure.cli.command_modules.documentdb._client_factory import (cf_documentdb)

cli_command(__name__, 'documentdb show', 'azure.cli.command_modules.documentdb.sdk.operations.database_accounts_operations#DatabaseAccountsOperations.get', cf_documentdb)

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

It would also be better to extract out the base string to a variable like so:

custom_path = 'azure.cli.command_modules.documentdb.sdk.operations.database_accounts_operations#{}'

cli_command(__name__, 'documentdb list', custom_path.format('DatabaseAccountsOperations.list_by_resource_group'), cf_documentdb)
"""Create new DocumentDB Database Account
:param resource_group_name: Name of resource group
:param name: Name of DocumentDB Database Account
:param location: Resource group location

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

If these are in the params file, you don't need them here. These values will be ignored.

:param location: Resource group location
"""
consistency_policy = None
if not default_consistency_level is None:

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

More idiomatic Python would be:
if default_consistency_level is not None:

consistency_policy = ConsistencyPolicy(default_consistency_level, max_staleness_prefix, max_interval_in_seconds)

params = DatabaseAccountCreateUpdateParameters(
resource_group_location,

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

Why is resource_group_location required? If you specify the resource_group_name, the backend should be able to determine what the location is. Regardless, since you specify the resource_group_name, you should just look up the resource_group_location instead of making it something the user has to know and set.

:vartype name: str
:ivar type: The type of Azure resource.
:vartype type: str
:param location: The location of the resource group to which the resource

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

Since this is autogenerated, this might be simple a swagger documentation issue. Is location the "location of the resource" (which is usually the case, and may differ from the resource group's location) or is this truly the "resource group's location"? If so, you can't exactly change it since the resource group's location can't be changed, which makes this a senseless parameter.

This comment has been minimized.

Copy link
@dmakwana

dmakwana Jan 27, 2017

Author Contributor

Currently this value is required for the backend. (which will get changed)

#pylint: disable=line-too-long

from azure.cli.core.commands import cli_command
import azure.cli.command_modules.documentdb

This comment has been minimized.

Copy link
@derekbekoe

derekbekoe Jan 23, 2017

Member

Shouldn't import from azure.cli.command_modules.documentdb as there's a big perf. hit.
Is this needed in order to load the commands?

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 23, 2017

Member

He has the "load_commands" method, so it shouldn't be.

@dmakwana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2017

For the sake of consistency, the team wants to keep it 'documentdb'.

@dmakwana

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2017

FAILOVER PRIORITY CHANGE OUTPUT

(env) C:\Workspace\azure-cli>az documentdb failover-priority-change -h

Command
az documentdb failover-priority-change: Changes the failover priority for the Azure DocumentDB
database account.
A failover priority of 0 indicates a write region. The maximum value for a failover priority
= (total number of regions - 1). Failover priority values must be unique for each of the
regions in which the database account exists.

Arguments
--failover-policies: Space separated failover policies in 'regionName=failoverPriority' format.
E.g "East US"=0.

Resource Id Arguments
--ids : One or more resource IDs. If provided, no other 'Resource Id' arguments
should be specified.
--name -n : Name of the DocumentDB Database Account.
--resource-group -g: Name of resource group.


CREATE OUTPUT

(env) C:\Workspace\azure-cli>az documentdb create -h

Command
az documentdb create: Create a new Azure DocumentDB database account.

Arguments
--locations [Required]: Space separated locations in 'regionName=failoverPriority'
format. E.g "East US"=0. Failover priority values are 0 for
write regions and greater than 0 for read regions. A failover
priority value must be unique and less than the total number of
regions.
--name -n [Required]: Name of the DocumentDB Database Account.
--resource-group -g [Required]: Name of the resource group.
--default-consistency-level : Default consistency level of the DocumentDB Database account.
Allowed values: BoundedStaleness, Eventual, Session, Strong.
--ip-range-filter : Firewall support. Specifies the set of IP addresses or IP
address ranges in CIDR form to be included as the allowed list
of client IPs for a given database account. IP addresses/ranges
must be comma separated and must not contain any spaces.
--max-interval : When used with Bounded Staleness consistency, this value
represents the time amount of staleness (in seconds) tolerated.
Accepted range for this value is 1 - 100. Default: 5.
--max-staleness-prefix : When used with Bounded Staleness consistency, this value
represents the number of stale requests tolerated. Accepted
range for this value is 1 - 2,147,483,647. Default: 100.


REGENERATE KEY OUTPUT

(env) C:\Workspace\azure-cli>az documentdb regenerate-key -h

Command
az documentdb regenerate-key: Regenerates an access key for the specified Azure DocumentDB
database account.

Arguments
--key-kind [Required]: The access key to regenerate. Allowed values: primary, primaryReadonly,
secondary, secondaryReadonly.

Resource Id Arguments
--ids : One or more resource IDs. If provided, no other 'Resource Id' arguments
should be specified.
--name -n : Name of the DocumentDB Database Account.
--resource-group -g : Name of resource group.

@tjprescott
Copy link
Member

left a comment

Overall looking much better. Please add the validator to 'ip_range_filter', fix the alias for 'max_interval' and show what the module loading time is (you can see it by running a command with --debug) and I'd be happy to approve and merge this and create a new issue with any additional clean-up work.

register_cli_argument('documentdb create', 'ip_range_filter', help="IP Range Filter")
register_cli_argument('documentdb create', 'default_consistency_level', help="default consistency level of the DocumentDB Database account", **enum_choice_list(DefaultConsistencyLevel))
register_cli_argument('documentdb create', 'max_staleness_prefix', help="when used with Bounded Staleness consistency, this value represents the number of stale requests tolerated. Accepted range for this value is 1 - 2,147,483,647", type=int)
register_cli_argument('documentdb create', 'max_interval', help="when used with Bounded Staleness consistency, this value represents the time amount of staleness (in seconds) tolerated. Accepted range for this value is 1 - 100", type=int)

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 30, 2017

Member

Since the parameter is still called "max_interval_in_seconds" (in custom.py) then the correct registration should be:

register_cli_argument('documentdb create', 'max_interval_in_seconds', options_list=('--max-interval',), help='...', type=int)

Otherwise, this alias will be applied to nothing.

consistency_policy = None
if not default_consistency_level is None:
if default_consistency_level is not None:
consistency_policy = ConsistencyPolicy(default_consistency_level, max_staleness_prefix, max_interval_in_seconds)

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 30, 2017

Member

Also, for clarity in your help text, for these three parameters, you could add the arg_group='Consistency Policy' kwarg to their register_cli_argument lines. This would cause these parameters to be visually grouped in help. Up to you whether you think that would enhance clarity or not, but wanted to point out the option.

fp_dict.append(FailoverPolicy(comps[0], int(comps[1])))
ns.failover_policies = fp_dict

def validate_locations(ns):

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 30, 2017

Member

since these are identical in logic, you could just have "validate_failover_policies_or_locations" or something. If in the future you think they might be handled differently, then leaving them like this is fine. If they will always move together, then this becomes an added maintenance burden for your team.

This comment has been minimized.

Copy link
@dmakwana

dmakwana Jan 30, 2017

Author Contributor

Yes I believe the 'location' object could change in the future so I will leave it as is


register_cli_argument('documentdb create', 'account_name', completer=None)
register_cli_argument('documentdb create', 'resource_group_name', help="name of the resource group")
register_cli_argument('documentdb create', 'resource_group_location', ignore_type, help="location of the resource group")

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 30, 2017

Member

If you are using 'ignore_type' it makes more sense to remove the 'help' kwarg here. It will be ignored, which would be confusing to someone reading your code.

register_cli_argument('documentdb create', 'default_consistency_level', help="default consistency level of the DocumentDB Database account", **enum_choice_list(DefaultConsistencyLevel))
register_cli_argument('documentdb create', 'max_staleness_prefix', help="when used with Bounded Staleness consistency, this value represents the number of stale requests tolerated. Accepted range for this value is 1 - 2,147,483,647", type=int)
register_cli_argument('documentdb create', 'max_interval', help="when used with Bounded Staleness consistency, this value represents the time amount of staleness (in seconds) tolerated. Accepted range for this value is 1 - 100", type=int)
register_cli_argument('documentdb create', 'ip_range_filter', help="firewall support. Specifies the set of IP addresses or IP address ranges in CIDR form to be included as the allowed list of client IPs for a given database account. IP addresses/ranges must be comma separated and must not contain any spaces")

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 30, 2017

Member

We do not accept comma separated lists anywhere in the CLI. All of our lists are space-separated. If the SDK accepts a string with comma separated values, then there should be a validator on this parameter that takes the list of space-separated values and joins them with ','.

register_cli_argument('documentdb create', 'locations', validator=validate_locations, help="space separated locations in 'regionName=failoverPriority' format. E.g \"East US\"=0. Failover priority values are 0 for write regions and greater than 0 for read regions. A failover priority value must be unique and less than the total number of regions.", nargs='+')
register_cli_argument('documentdb create', 'default_consistency_level', help="default consistency level of the DocumentDB Database account", **enum_choice_list(DefaultConsistencyLevel))
register_cli_argument('documentdb create', 'max_staleness_prefix', help="when used with Bounded Staleness consistency, this value represents the number of stale requests tolerated. Accepted range for this value is 1 - 2,147,483,647", type=int)
register_cli_argument('documentdb create', 'max_interval', help="when used with Bounded Staleness consistency, this value represents the time amount of staleness (in seconds) tolerated. Accepted range for this value is 1 - 100", type=int)
register_cli_argument('documentdb create', 'ip_range_filter', help="firewall support. Specifies the set of IP addresses or IP address ranges in CIDR form to be included as the allowed list of client IPs for a given database account. IP addresses/ranges must be comma separated and must not contain any spaces")
register_cli_argument('documentdb create', 'ip_range_filter', validator=validate_ip_range_filter, help="firewall support. Specifies the set of IP addresses or IP address ranges in CIDR form to be included as the allowed list of client IPs for a given database account. IP addresses/ranges must be comma separated and must not contain any spaces")

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 30, 2017

Member

This needs the following kwarg nargs='+'

@@ -33,16 +33,19 @@ def validate_locations(ns):
loc_dict.append(Location(location_name=comps[0], failover_priority=int(comps[1])))
ns.locations = loc_dict

def validate_ip_range_filter(ns):
ns.ip_range_filter = ",".join(ns.ip_range_filter.split())

This comment has been minimized.

Copy link
@tjprescott

tjprescott Jan 30, 2017

Member

If the range filter isn't specified, this will crash.

def validate_ip_range_filter(ns):
  if ns.ip_range_filter:
    ns.ip_range_filter = ','.join(ns.ip_range_filter)

With nargs=+ specified in the parameter registration, this property will already contain a list of strings, so no split is necessary. Using split would require a space separate list of filters all surrounded by quotes ("filter1 filter2 ..."), which is also not a pattern we do not permit.

dmakwana added some commits Jan 30, 2017

@lmazuel

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

@tjprescott @dmakwana FYI I released the azure-mgmt-documentdb package
https://pypi.python.org/pypi/azure-mgmt-documentdb

@tjprescott

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

@dmakwana probably best to just remove the embedded SDK and update the imports then.

@tjprescott tjprescott merged commit 9a0fd7d into Azure:master Feb 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.