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.

Sign up for GitHub

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

Added support for track 2 SDKs (azure-mgmt-iothub >= 1.0.0) #329

Merged
merged 10 commits into from
Apr 22, 2021
Merged

Added support for track 2 SDKs (azure-mgmt-iothub >= 1.0.0) #329

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d5dce83
Added support for track 2 SDKs (azure-mgmt-iothub >= 1.0.0)
c-ryan-k Apr 13, 2021
53c6718
whitespace fix
c-ryan-k Apr 16, 2021
d07576b
Merge branch 'azure-dev' into sdk_track_2_support
c-ryan-k Apr 16, 2021
6bdb721
Added packaging dependency in setup.py
c-ryan-k Apr 19, 2021
2a58430
Minor discovery and test updates
c-ryan-k Apr 19, 2021
5b2d66d
PR feedback and discovery parameter fix
c-ryan-k Apr 21, 2021
7433c2c
Fix discovery resource group variable references
c-ryan-k Apr 21, 2021
5efdd3d
Disable exception type-checking on discovery.find_iothub
c-ryan-k Apr 22, 2021
a39d4ac
whitespace fix
c-ryan-k Apr 22, 2021
b45cd89
Merge branch 'dev' into sdk_track_2_support
c-ryan-k Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions azext_iot/common/utility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,10 +438,14 @@ def is_iso8601_time(self, to_validate: str) -> bool:
return False


def ensure_min_version(cur_ver, min_ver):
from pkg_resources._vendor.packaging import version
def ensure_min_version(min_ver):
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice either through docstrings and/or slightly altering the name that this is to ensure the min version of the iothub mgmt sdk.

from packaging import version
try:
from azure.mgmt.iothub import __version__ as iot_sdk_version
except ImportError:
from azure.mgmt.iothub._configuration import VERSION as iot_sdk_version

return version.parse(cur_ver) >= version.parse(min_ver)
return version.parse(iot_sdk_version) >= version.parse(min_ver)


def scantree(path):
Expand Down
3 changes: 3 additions & 0 deletions azext_iot/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,6 @@

# Config Key's
CONFIG_KEY_UAMQP_EXT_VERSION = "uamqp_ext_version"

# Initial Track 2 SDK version
TRACK_2_SDK_MIN_VERSION = '1.0.0'
Copy link
Member

Choose a reason for hiding this comment

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

We should prefix this with IOTHUB_ or re-organize like creating dictionary that holds IoT Hub constants

41 changes: 26 additions & 15 deletions azext_iot/iothub/providers/discovery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@

from knack.util import CLIError
from knack.log import get_logger
from azext_iot.common.utility import trim_from_start
from azure.cli.core.commands.client_factory import get_subscription_id
from azext_iot.common.utility import trim_from_start, ensure_min_version
from azext_iot.iothub.models.iothub_target import IotHubTarget
from azext_iot._factory import iot_hub_service_factory
from azext_iot.constants import TRACK_2_SDK_MIN_VERSION
from typing import Dict, List
from enum import Enum, EnumMeta

PRIVILEDGED_ACCESS_RIGHTS_SET = set(
["RegistryWrite", "ServiceConnect", "DeviceConnect"]
Expand All @@ -30,9 +33,9 @@ def _initialize_client(self):
if not self.client:
if getattr(self.cmd, "cli_ctx", None):
self.client = iot_hub_service_factory(self.cmd.cli_ctx)
self.sub_id = get_subscription_id(self.cmd.cli_ctx)
else:
self.client = self.cmd
self.sub_id = self.client.config.subscription_id
Copy link
Member

Choose a reason for hiding this comment

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

We may hit a regression here since before sub_id was extracted when the client was initialized and used to build the target dictionary. Might help determine problems if we change this https://github.com/c-ryan-k/azure-iot-cli-extension/blob/sdk_track_2_support/azext_iot/tests/iothub/test_iothub_discovery_int.py#L102 to assert sub_id is not the "unknown" placeholder value.


def get_iothubs(self, rg: str = None) -> List:
self._initialize_client()
Expand All @@ -44,11 +47,15 @@ def get_iothubs(self, rg: str = None) -> List:
else:
hubs_pager = self.client.list_by_resource_group(resource_group_name=rg)

try:
while True:
hubs_list.extend(hubs_pager.advance_page())
except StopIteration:
pass
if ensure_min_version(TRACK_2_SDK_MIN_VERSION):
for hubs in hubs_pager.by_page():
hubs_list.extend(hubs)
else:
try:
while True:
hubs_list.extend(hubs_pager.advance_page())
except StopIteration:
pass

return hubs_list

Expand All @@ -60,23 +67,27 @@ def get_policies(self, hub_name: str, rg: str) -> List:
)
policy_list = []

try:
while True:
policy_list.extend(policy_pager.advance_page())
except StopIteration:
pass
if ensure_min_version(TRACK_2_SDK_MIN_VERSION):
for policy in policy_pager.by_page():
policy_list.extend(policy)
else:
try:
while True:
policy_list.extend(policy_pager.advance_page())
except StopIteration:
pass

return policy_list

def find_iothub(self, hub_name: str, rg: str = None):
self._initialize_client()

from azure.mgmt.iothub.models import ErrorDetailsException
from azure.core.exceptions import HttpResponseError

if rg:
try:
return self.client.get(resource_group_name=rg, resource_name=hub_name)
except ErrorDetailsException:
except HttpResponseError:
Copy link
Member

@digimaun digimaun Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Will this exception work for both SDK types?

Copy link
Member

Choose a reason for hiding this comment

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

Btw python supports an exception tuple...for example except (ErrorDetailsException, HttpResponseError):. But one exception type would be preferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, this is an issue I was working on yesterday - the azure.core exception import will cause an issue with the older CLI (2.3.1) - I'll have an update on this in a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

So just to follow up, this issue is a bit complex.

  • We switched from ErrorDetailsException to the azure.core.HttpResponseError (base error class) as ErrorDetailsException no longer exists in our Track 2 SDKs.
  • In AZ CLI 2.3.1, however - we don't have the azure.core exceptions
  • The actual exception this will return is a ResourceNotFoundError which also comes from azure.core

Is it pertinent here to catch the specific error type if we're not utilizing the error/exception itself?
I'd like to avoid blanket except: statements (or more try/catch imports) but that might be the easiest fix here.

Copy link
Member

Choose a reason for hiding this comment

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

Good details...my initial thought was if there was a common base exception like Exception we could use but I think the most pragmatic solution is the one you suggest in this specific case having except: since we are not using the exception itself.

raise CLIError(
"Unable to find IoT Hub: {} in resource group: {}".format(
hub_name, rg
Expand Down Expand Up @@ -186,7 +197,7 @@ def _build_target(
target["subscription"] = self.sub_id
target["resourcegroup"] = iothub.additional_properties.get("resourcegroup")
target["location"] = iothub.location
target["sku_tier"] = iothub.sku.tier.value
target["sku_tier"] = iothub.sku.tier.value if isinstance(iothub.sku.tier, (Enum, EnumMeta)) else iothub.sku.tier

if include_events:
events = {}
Expand Down
9 changes: 4 additions & 5 deletions azext_iot/operations/hub.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import six
from knack.log import get_logger
from knack.util import CLIError
from enum import Enum, EnumMeta
from azext_iot.constants import (
EXTENSION_ROOT,
DEVICE_DEVICESCOPE_PREFIX,
Expand Down Expand Up @@ -2287,7 +2288,6 @@ def iot_device_export(
resource_group_name=None,
):
from azext_iot._factory import iot_hub_service_factory
from azure.mgmt.iothub import __version__ as iot_sdk_version

client = iot_hub_service_factory(cmd.cli_ctx)
discovery = IotHubDiscovery(cmd)
Expand All @@ -2298,7 +2298,7 @@ def iot_device_export(
if exists(blob_container_uri):
blob_container_uri = read_file_content(blob_container_uri)

if ensure_min_version(iot_sdk_version, "0.12.0"):
if ensure_min_version("0.12.0"):
from azure.mgmt.iothub.models import ExportDevicesRequest
from azext_iot.common.shared import AuthenticationType

Expand Down Expand Up @@ -2336,7 +2336,6 @@ def iot_device_import(
resource_group_name=None,
):
from azext_iot._factory import iot_hub_service_factory
from azure.mgmt.iothub import __version__ as iot_sdk_version

client = iot_hub_service_factory(cmd.cli_ctx)
discovery = IotHubDiscovery(cmd)
Expand All @@ -2350,7 +2349,7 @@ def iot_device_import(
if exists(output_blob_container_uri):
output_blob_container_uri = read_file_content(output_blob_container_uri)

if ensure_min_version(iot_sdk_version, "0.12.0"):
if ensure_min_version("0.12.0"):
from azure.mgmt.iothub.models import ImportDevicesRequest
from azext_iot.common.shared import AuthenticationType

Expand Down Expand Up @@ -2711,7 +2710,7 @@ def _get_hub_connection_string(
entityPath,
)
for p in policies
if "serviceconnect" in p.rights.value.lower()
if "serviceconnect" in (p.rights.value.lower() if isinstance(p.rights, (Enum, EnumMeta)) else p.rights.lower())
]

hostname = hub.properties.host_name
Expand Down
15 changes: 15 additions & 0 deletions azext_iot/tests/iothub/jobs/test_iothub_jobs_int.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,21 @@ def test_jobs(self):
checks=[self.check("jobId", self.job_ids[2])],
)

# Allow time for job to transfer to scheduled state (cannot cancel job in running state)
from time import sleep
sleep(5)
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this longer? Or is 5 second sleep consistently working.

Copy link
Member Author

Choose a reason for hiding this comment

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

Consistently working for me, but I also haven't seen this issue pop up in the build pipelines, only in local testing - one of my hubs got stuck with a job pending in a scheduled state, and couldn't add a new job.

The error was because our test tried to cancel a job before it transitioned from running to scheduled and this seemed to fix it for me (at least for now)


self.cmd(
"iot hub job show --job-id {} -n {} -g {}".format(
self.job_ids[2], LIVE_HUB, LIVE_RG
),
checks=[
self.check("jobId", self.job_ids[2]),
self.check("status", "scheduled"),
],
)

# Cancel job
self.cmd(
"iot hub job cancel --job-id {} -n {} -g {}".format(
self.job_ids[2], LIVE_HUB, LIVE_RG
Expand Down
9 changes: 7 additions & 2 deletions azext_iot/tests/utility/test_iot_utility_unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,13 @@ class TestVersionComparison(object):
("2.0.1.9", "2.0.6", False),
],
)
def test_ensure_min_version(self, current, minimum, expected):
assert ensure_min_version(current, minimum) == expected
def test_ensure_min_version(self, mocker, current, minimum, expected):
try:
mocker.patch("azure.mgmt.iothub.__version__", current)
except:
mocker.patch("azure.mgmt.iothub._configuration.VERSION", current)

assert ensure_min_version(minimum) == expected


class TestEmbeddedCli(object):
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
# though that is installed out of band (managed by the extension)
# for compatibility reasons.

DEPENDENCIES = ["paho-mqtt==1.5.0", "jsonschema==3.2.0", "setuptools"]
DEPENDENCIES = ["paho-mqtt==1.5.0", "jsonschema==3.2.0", "setuptools", "packaging"]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need setuptools anymore?



CLASSIFIERS = [
Expand Down