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

c-ryan-k · 2021-04-16T21:27:35Z

This project has adopted the Microsoft Open Source Code of Conduct. For more information see the Code of Conduct FAQ or contact [email protected] with any additional questions or comments.

Thank you for contributing to the IoT extension!

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

If introducing new functionality or modified behavior, are they backed by unit and integration tests?
In the same context as above are command names and their parameter definitions accurate? Do help docs have sufficient content?
Have all unit and integration tests passed locally? i.e. pytest <project root> -vv
Have static checks passed using the .pylintrc and .flake8 rules? Look at the CI scripts for example usage.
Have you made an entry in HISTORY.rst which concisely explains your feature or change?

c-ryan-k · 2021-04-16T22:54:55Z

Will need to add an explicit dependency for packaging in setup.py, but waiting for #328 to get merged so I can clean up the conflicts

digimaun · 2021-04-20T00:52:16Z

setup.py

@@ -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"]


Do we need setuptools anymore?

digimaun · 2021-04-20T00:52:57Z

azext_iot/tests/iothub/jobs/test_iothub_jobs_int.py

@@ -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)


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

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)

digimaun · 2021-04-20T00:59:35Z

azext_iot/constants.py

@@ -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'


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

digimaun · 2021-04-20T01:04:50Z

azext_iot/common/utility.py

@@ -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):


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.

digimaun · 2021-04-20T01:29:43Z

azext_iot/iothub/providers/discovery.py


        if rg:
            try:
                return self.client.get(resource_group_name=rg, resource_name=hub_name)
-            except ErrorDetailsException:
+            except HttpResponseError:


Will this exception work for both SDK types?

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

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.

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.

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.

digimaun · 2021-04-20T01:33:31Z

azext_iot/iothub/providers/discovery.py

            else:
                self.client = self.cmd
-            self.sub_id = self.client.config.subscription_id


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.

c-ryan-k added 2 commits April 16, 2021 14:17

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

d5dce83

whitespace fix

53c6718

c-ryan-k added 3 commits April 16, 2021 16:59

Merge branch 'azure-dev' into sdk_track_2_support

d07576b

Added packaging dependency in setup.py

6bdb721

Minor discovery and test updates

2a58430

c-ryan-k marked this pull request as ready for review April 19, 2021 21:34

c-ryan-k requested a review from digimaun as a code owner April 19, 2021 21:34

digimaun reviewed Apr 20, 2021

View reviewed changes

c-ryan-k added 5 commits April 21, 2021 15:33

PR feedback and discovery parameter fix

5b2d66d

Fix discovery resource group variable references

7433c2c

Disable exception type-checking on discovery.find_iothub

5efdd3d

whitespace fix

a39d4ac

Merge branch 'dev' into sdk_track_2_support

b45cd89

digimaun approved these changes Apr 22, 2021

View reviewed changes

digimaun merged commit 0c9f0f4 into Azure:dev Apr 22, 2021

c-ryan-k deleted the sdk_track_2_support branch August 27, 2021 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

c-ryan-k commented Apr 16, 2021

c-ryan-k commented Apr 16, 2021

digimaun Apr 20, 2021

digimaun Apr 20, 2021

c-ryan-k Apr 20, 2021

digimaun Apr 20, 2021

digimaun Apr 20, 2021

digimaun Apr 20, 2021 •

edited

Loading

digimaun Apr 20, 2021

c-ryan-k Apr 20, 2021

c-ryan-k Apr 21, 2021

digimaun Apr 22, 2021

digimaun Apr 20, 2021

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

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

Conversation

c-ryan-k commented Apr 16, 2021

General Guidelines

c-ryan-k commented Apr 16, 2021

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

digimaun Apr 20, 2021 • 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

digimaun Apr 20, 2021 •

edited

Loading