-
Notifications
You must be signed in to change notification settings - Fork 382
feature (azure-iot-device): Add 'sastoken_ttl' kwarg option to factory methods #579
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
cartertinney
commented
May 29, 2020
- Added support for custom SasToken TTL via the 'sastoken_ttl' kwarg on SAS-based factory methods of the IoTHub clients
- Added support for custom SasToken TTL via the 'sastoken_ttl' kwarg on SAS-based facotry methods of the Provisioning clients
| client_class.create_from_x509_certificate( | ||
| x509=x509, hostname=self.hostname, device_id=self.device_id, sastoken_ttl=1000 | ||
| ) | ||
|
|
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.
There is a better pattern for handling excluded kwargs from a subset of factory methods, but I'll wait to implement it until we have more than one instance of this problem
| sastoken_mock = mocker.patch.object(st, "SasToken") | ||
| expected_uri = "{hostname}/devices/{device_id}".format( | ||
| hostname=self.hostname, device_id=self.device_id | ||
| ) |
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.
We test SasToken config here, as opposed in the shared user option tests, because this isn't a kwarg that goes to the PipelineConfig. I'll probably rename SharedIoTHubClientCreateMethodUserOptionTests down the road to reflect that, but I want to see how this pattern develops as we add more kwargs
|
./azp run Azure.azure-iot-sdk-python-dps-e2e |
|
/azp run Azure.azure-iot-sdk-python-dps-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| :param proxy_options: Options for sending traffic through proxy servers. | ||
| :type proxy_options: :class:`azure.iot.device.ProxyOptions` | ||
| :param int sastoken_ttl: The time to live (in seconds) for the created SasToken used for | ||
| authentication. Default is 3600 seconds (1 hour) |
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.
random comment: I like to see the level of detail in our docstrings go up. If you haven't looked at the public-facing docstrings in the other Azure SDKs, you should (I think I'm thinking of eventhub, but maybe uamqp).
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.
Yeah I agree, we could do a bit more. Once we get docgen occurring correctly, there's a number of improvements I think could be made.
BertKleewein
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.
![]()