-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[DataBoxEdge] new command module: support for data-box-edge devices and management #16193
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
Conversation
|
Databoxedge |
haroldrandom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core. LGTM.
BTW, would DataboxEdge be better?
| MGMT_CONSUMPTION = ('azure.mgmt.consumption', None) | ||
| MGMT_CONTAINERINSTANCE = ('azure.mgmt.containerinstance', None) | ||
| MGMT_COSMOSDB = ('azure.mgmt.cosmosdb', None) | ||
| MGMT_DATABOXEDGE = ('azure.mgmt.databoxedge', None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could specify the client name here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
|
|
||
| def __init__(self, cli_ctx=None): | ||
| from azure.cli.core.commands import CliCommandType | ||
| from .generated._client_factory import cf_databoxedge_cl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you still want to use generated code, right?
and we should avoid to use relative import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
src/azure-cli/azure/cli/command_modules/databoxedge/generated/_client_factory.py
Show resolved
Hide resolved
|
|
||
| helps['databoxedge device install-update'] = """ | ||
| type: command | ||
| short-summary: "Installs the updates on the Data Box Edge/Data Box Gateway device." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installs -> Install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have overwrite all helps under manual folder
| helps['databoxedge'] = """ | ||
| type: group | ||
| short-summary: Manage node with databoxedge | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
| c.argument('tags', tags_type) | ||
| c.argument('sku', action=AddSku, nargs='+', help='The SKU type.') | ||
| c.argument('etag', type=str, help='The etag for the devices.') | ||
| c.argument('data_box_edge_device_status', options_list=['--status'], arg_type=get_enum_type(['ReadyToSetup', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we get the enum type from sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have asked @qiaozha for support, guess developing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is enum definition in SDK, you could rewrite it now.
Getting from SDK could adjust with api version update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| type: command | ||
| short-summary: "Downloads the updates on a Data Box Edge/Data Box Gateway device." | ||
| examples: | ||
| - name: DownloadUpdatesPost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
such example name is not friendly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have overwrite all helps under manual folder
|
|
||
| helps['databoxedge device download-update'] = """ | ||
| type: command | ||
| short-summary: "Downloads the updates on a Data Box Edge/Data Box Gateway device." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious about the download-update work. which kind of updates could be downloaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Databoxedge device has many installed pkgs, so CX can run scan-for-update first, and then show-update-summary to check how many pkgs they can update. After download according to the summary using download-update, they can run install-update to finish updating the whole device.
So for you question, they actually download the updated pkg.
src/azure-cli/azure/cli/command_modules/databoxedge/generated/_params.py
Show resolved
Hide resolved
| c.argument('start', type=str, help='The start time of the schedule in UTC.') | ||
| c.argument('stop', type=str, help='The stop time of the schedule in UTC.') | ||
| c.argument('rate_in_mbps', type=int, help='The bandwidth rate in Mbps.') | ||
| c.argument('days', nargs='+', help='The days of the week when this schedule is applicable.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the days parameter is list array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users can create same bandwidth rule for several days, for example, 10mbps between 09:00:00 and 18:00:00 for both 'Saturday' and 'Sunday'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have test to cover such scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test bandwidth-schedule, the device need to be registered which need a vm prepared and some manual work on web page. So it's hard to record tests. But I do test this scenario with the vm service team provided.
src/azure-cli/azure/cli/command_modules/databoxedge/generated/_params.py
Outdated
Show resolved
Hide resolved
| c.argument('shipping_address_city', options_list=['--city'], type=str, help='The city name.') | ||
| c.argument('shipping_address_state', options_list=['--state'], type=str, help='The state name.') | ||
| c.argument('shipping_address_country', options_list=['--country'], type=str, help='The country name.') | ||
| c.argument('contact_information_contact_person', options_list=['--contact-person'], type=str, help='The ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
group contact related arguments, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
| - name: --sku | ||
| short-summary: "The SKU type." | ||
| long-summary: | | ||
| Usage: --sku name=XX tier=XX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider if we could specify sku with only one property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sku name can be 'Gateway' or 'Edge', the tier can be 'Standard' or something else. How can we specify with only one property?
src/azure-cli/azure/cli/command_modules/databoxedge/generated/commands.py
Outdated
Show resolved
Hide resolved
| databoxedge_device = CliCommandType( | ||
| operations_tmpl='azure.mgmt.databoxedge.operations._devices_operations#DevicesOperations.{}', | ||
| client_factory=cf_device) | ||
| with self.command_group('databoxedge device', databoxedge_device, client_factory=cf_device) as g: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add resource type and min_api for commands or arguments to make sure unsupported commands hidden in unsupported environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
src/azure-cli/azure/cli/command_modules/databoxedge/generated/commands.py
Show resolved
Hide resolved
| short-summary: "Gets all the Data Box Edge/Data Box Gateway devices in a resource group. And Gets all the Data Box \ | ||
| Edge/Data Box Gateway devices in a subscription." | ||
| examples: | ||
| - name: DataBoxEdgeDeviceGetByResourceGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we refine the example name to be more friendly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all refined under /manual/_help.py
src/azure-cli/azure/cli/command_modules/databoxedge/tests/hybrid_2019_03_01/example_steps.py
Show resolved
Hide resolved
| Usage: --sku name=XX tier=XX | ||
| name: SKU name. | ||
| tier: The SKU tier. This is based on the SKU name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sku issue, it seems the tier is based on name. So I am thinking if we could get sku type with name only.
With current design, it is a little complicated for customer to define their sku and they have no idea for allowed value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. But please see https://github.com/Azure/azure-cli/pull/16193/files#r547053949
Description
CLI support for databoxedge
This PR covers databoxedge operations which not involving encryption flow, including:
az databoxedge list-sku|list-node|show-jobaz databoxedge device create|list|show|update|deleteaz databoxedge device scan-for-update|show-update-summary|download-update|install-updateaz databoxedge alert list|showaz databoxedge order create|list|show|update|deleteaz databoxedge bandwidth-schedule create|list|show|update|deleteTesting Guide
Commands which don't need the device to be registered
See test cases
Commands which need the device registered
Preparation: Finish tutorial on prepare, install and activate