Skip to content
Merged
13 changes: 10 additions & 3 deletions azext_iot/central/commands_device.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from knack.util import CLIError
from azext_iot.central.providers import CentralDeviceProvider
from azext_iot.common.shared import DeviceStatus
Comment thread
valluriraj marked this conversation as resolved.
Outdated


def list_devices(
Expand Down Expand Up @@ -68,10 +69,16 @@ def registration_info(
device_id=None,
token=None,
central_dns_suffix="azureiotcentral.com",
device_status=None,
):
provider = CentralDeviceProvider(cmd=cmd, app_id=app_id, token=token)
provider = CentralDeviceProvider(cmd=cmd, app_id=app_id, token=token,)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a , at the end?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use an auto-formatter called "Black", that is adding this additional comma at the end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though in this particular case it should probably be removed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not worry about this one. This is standard python black behavior, in this case adding a comma after the last kwarg element. If you remove it now (which is fine), the next change to this module and a re-run of the formatter would re-introduce it.

python black is an opinionated formatter with minimal configuration. Idea for using Black is to save time and mental energy for more important matters. By using it, you agree to cede control over minutiae of hand-formatting. Also Black will check that the reformatted code still produces a valid AST that is equivalent to the original. It's a relatively newer Python tool that has immense popularity in the open source world.

if not device_id:
return provider.get_all_registration_info(central_dns_suffix=central_dns_suffix)
return provider.get_all_registration_info(
central_dns_suffix=central_dns_suffix, device_status=device_status

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What pattern are we following, new line for parameter or on the same line?

@ghost ghost Apr 30, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use an auto-formatter called "Black", that tool is deciding the spacing/line breaks for us

)

return provider.get_device_registration_info(
device_id=device_id, central_dns_suffix=central_dns_suffix
device_id=device_id,
central_dns_suffix=central_dns_suffix,
device_status=device_status,
)
9 changes: 8 additions & 1 deletion azext_iot/central/params.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
CLI parameter definitions.
"""

from azure.cli.core.commands.parameters import get_three_state_flag
from azure.cli.core.commands.parameters import get_three_state_flag, get_enum_type
from azext_iot.common.shared import DeviceStatus


def load_central_arguments(self, _):
Expand Down Expand Up @@ -60,3 +61,9 @@ def load_central_arguments(self, _):
help="Central dns suffix. "
"This enables running cli commands against non public/prod environments",
)
context.argument(
"device_status",
options_list=["--devicestatus", "-ds"],
Comment thread
valluriraj marked this conversation as resolved.
Outdated
arg_type=get_enum_type(DeviceStatus),
help="Indicates filter option for device status",
)
91 changes: 85 additions & 6 deletions azext_iot/central/providers/device_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from knack.log import get_logger
from azext_iot.central import services as central_services
from azext_iot.dps.services import global_service as dps_global_service
from azext_iot.common.shared import DeviceStatus

logger = get_logger(__name__)

Expand Down Expand Up @@ -170,11 +171,15 @@ def get_device_credentials(
return credentials

def get_device_registration_info(
self, device_id, central_dns_suffix="azureiotcentral.com",
self, device_id, device_status, central_dns_suffix="azureiotcentral.com"
Comment thread
valluriraj marked this conversation as resolved.
Outdated
):
info = self._device_registration_info.get(device_id)

if not info:
central_info = self.get_device(device_id)
central_info = self.update_device_info_filter_status(
Comment thread
valluriraj marked this conversation as resolved.
Outdated
central_info, device_status
)
credentials = self.get_device_credentials(
device_id=device_id, central_dns_suffix=central_dns_suffix
)
Expand All @@ -183,7 +188,9 @@ def get_device_registration_info(
dps_state = dps_global_service.get_registration_state(
id_scope=id_scope, key=key, device_id=device_id
)
central_info = self.get_device(device_id)
dps_state = self.filter_dps_info(
dps_state, central_info.get("deviceStatus")
)
info = {
"@device_id": device_id,
"dps_state": dps_state,
Expand All @@ -194,20 +201,92 @@ def get_device_registration_info(

return info

def get_all_registration_info(self, central_dns_suffix="azureiotcentral.com"):
def filter_dps_info(self, dps_info, device_status):
Comment thread
valluriraj marked this conversation as resolved.
Outdated
Comment thread
valluriraj marked this conversation as resolved.
Outdated
error = {
"provisioned": "None",
"registered": "Device it not yet provisioned.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: Device is not yet provisioned.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The registered value string has a period at the end . It would be good to standardize on ending with a period or not.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest that cleaning up error messages is a P2 since I have a task in our backlog to clean up all error messages once we are ready to ship.

Let me know if this is something PR blocking and I can work with you to resolve

"blocked": "Device is blocked by admin",
"unassociated": "Device does not have a valid template associated with it",
}

filtered_dps_info = {
"status": dps_info.get("status"),
}

filtered_dps_info.update({"error": error.get(device_status)})
Comment thread
valluriraj marked this conversation as resolved.
Outdated
return filtered_dps_info

def update_device_info_filter_status(self, device, value):
Comment thread
valluriraj marked this conversation as resolved.
Outdated
if value is None:
Comment thread
valluriraj marked this conversation as resolved.
Outdated
return self.update_device_status(device)
updated_device_data = {
"id": device["id"],
"displayName": device.get("displayName"),
Comment thread
valluriraj marked this conversation as resolved.
Outdated
"instanceOf": device.get("instanceOf"),
"simulated": device.get("simulated"),
"deviceStatus": value,
}
return updated_device_data

def update_device_status(self, device):
if device["approved"] is False:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say extract this to a function:

def determine_device_status(self, device):
  if (case1):
    return x
  if case2:
    return y
  ...

then just call:

device_status = self.determine_device_status(device)
return self.add_filter_status(device, device_status)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

infact, I think this code could be moved into the get_device code:

device = central_services.device.get_device(
                cmd=self._cmd,
                app_id=self._app_id,
                device_id=device_id,
                token=self._token,
                central_dns_suffix=central_dns_suffix,
            )
device_status = self.determine_device_status(device)
if device_status:
  device["deviceStatus"] = device_status

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or perhaps even at the service level

updated_device = self.update_device_info_filter_status(
device, DeviceStatus.blocked.value
)
else:
if device.get("instanceOf") is None:
updated_device = self.update_device_info_filter_status(
device, DeviceStatus.unassociated.value
)
else:
if device["provisioned"] is False:
Comment thread
valluriraj marked this conversation as resolved.
Outdated
updated_device = self.update_device_info_filter_status(
device, DeviceStatus.registered.value
)
else:
updated_device = self.update_device_info_filter_status(
device, DeviceStatus.provisioned.value
)
return updated_device

def update_all_device_status(self, devices):
Comment thread
valluriraj marked this conversation as resolved.
Outdated
filtered_device_list = []
for device in devices:
filtered_device_list.append(self.update_device_status(device))
return filtered_device_list

def filter_device_list(self, devices, device_status):
filtered_device_list = []
updated_device_list = self.update_all_device_status(devices)
if device_status is None:
return updated_device_list

for device in updated_device_list:
if device.get("deviceStatus") is device_status:
filtered_device_list.append(device)
return filtered_device_list

def get_all_registration_info(
self, device_status, central_dns_suffix="azureiotcentral.com"
):

logger.warning("This command may take a long time to complete execution.")
devices = self.list_devices(central_dns_suffix=central_dns_suffix)
real_devices = [
device for device in devices.values() if not device["simulated"]
]
if len(devices) != len(real_devices):

Comment thread
valluriraj marked this conversation as resolved.
filtered_devices = self.filter_device_list(real_devices, device_status)

if len(devices) != len(filtered_devices):
logger.warning(
"Getting registration info for following devices. "
"All other devices are simulated. "
Comment thread
valluriraj marked this conversation as resolved.
Outdated
"{}".format([device["id"] for device in real_devices])
"{}".format([device["id"] for device in filtered_devices])
)
result = [
self.get_device_registration_info(device["id"]) for device in real_devices
self.get_device_registration_info(device["id"], device["deviceStatus"])
for device in filtered_devices
]

return result
11 changes: 11 additions & 0 deletions azext_iot/common/shared.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,14 @@ class JobStatusType(Enum):
cancelled = "cancelled"
scheduled = "scheduled"
queued = "queued"


class DeviceStatus(Enum):
Comment thread
valluriraj marked this conversation as resolved.
Outdated
"""
Type of Device status.
"""

provisioned = "provisioned"
registered = "registered"
blocked = "blocked"
unassociated = "unassociated"