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

Jump to bottom

feat(samples): Add invoke method sample and Query tests. #14252

Merged
merged 1 commit into from
Aug 18, 2020
Merged

feat(samples): Add invoke method sample and Query tests. #14252

merged 1 commit into from
Aug 18, 2020

Conversation

vinagesh
Copy link
Member

@vinagesh vinagesh commented Aug 14, 2020 •
edited
Loading

  1. Added query e2e tests
  2. Added samples for invoking methods on device and module
  3. Renamed some methods to have async
  4. Added configureAwait wherever missing
  5. Generated recorded files for old and new tests
  6. Added a cleanup for samples in case of errors

@vinagesh vinagesh force-pushed the vinagesh/samples branch 2 times, most recently from ed103a6 to 8d37544 Compare August 18, 2020 00:00
vinagesh
vinagesh commented Aug 18, 2020
View reviewed changes
sdk/iot/Azure.Iot.Hub.Service/samples/IotHubClientSamples/CleanupHelper.cs

try
{
AsyncPageable<TwinData> asyncPageableResponse = hubClient.Devices.GetTwinsAsync();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where I felt having a list on Device Identities might have been helpful. Maybe something to revisit. If not the overloaded method, we can have some custom implementation to get all device identities. If there is a one-liner to do this already and i'm missing it, let me know.

vinagesh
vinagesh commented Aug 18, 2020
View reviewed changes
sdk/iot/Azure.Iot.Hub.Service/samples/IotHubClientSamples/CleanupHelper.cs
deviceTwins.Add(twin);
}

foreach (TwinData twin in deviceTwins)
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic assumes all devices have a twin. Let me know if that is incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently i'm doing

  1. List all twins
  2. Get device Ids from there
  3. Get every device identity using the id
  4. Delete every device identity

Copy link
Member Author

Choose a reason for hiding this comment

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

If there is not a better way to do this, we should discuss how we can expose List all device identities. Customers might have a similar scenario too.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a way around Step 1, 2 and 3 ; but instead of calling Step 4 in a for-each loop, we could also use the bulk API.
I remember people being unhappy about this in the track 1 client, so we could probably expose an API that combines 1, 2 and 3 and returns the result?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should be careful in adding functionality that extends beyond the REST API. I'm not saying we don't, but let's treat that decision with care and thoroughly discuss it.

@vinagesh vinagesh force-pushed the vinagesh/samples branch 3 times, most recently from a9ac987 to 1d26254 Compare August 18, 2020 03:03
@vinagesh vinagesh marked this pull request as draft August 18, 2020 03:05
@vinagesh vinagesh changed the title feat(samples): Add invoke method sample. feat(samples): Add invoke method sample and Query tests. Aug 18, 2020
@vinagesh vinagesh marked this pull request as ready for review August 18, 2020 03:47
@vinagesh vinagesh force-pushed the vinagesh/samples branch 3 times, most recently from 89a6b75 to 49072be Compare August 18, 2020 04:21
vinagesh
vinagesh commented Aug 18, 2020
View reviewed changes
sdk/iot/Azure.Iot.Hub.Service/samples/IotHubClientSamples/BulkDeviceIdentityLifecycleSamples.cs
@@ -98,6 +98,8 @@ public async Task CreateDeviceIdentitiesAsync(IEnumerable<DeviceIdentity> device
}
catch (Exception ex)
{
// Try to cleanup before exiting with fatal error.
await CleanupHelper.DeleteAllDevicesInHubAsync(IoTHubServiceClient);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to ensure we cleanup all devices created in the hub even if there was a fatal error.

vinagesh
vinagesh commented Aug 18, 2020
View reviewed changes
sdk/iot/Azure.Iot.Hub.Service/samples/IotHubClientSamples/Program.cs
@@ -49,6 +50,20 @@ public static async Task Main(string[] args)

var bulkDeviceIdentityLifecycleSamples = new BulkDeviceIdentityLifecycleSamples(hubClient);
await bulkDeviceIdentityLifecycleSamples.RunSampleAsync();

var bulkModuledentityLifecycleSamples = new BulkModuleIdentityLifecycleSamples(hubClient);
Copy link
Member Author

@vinagesh vinagesh Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I had forgotten to include this in my previous PR

vinagesh
vinagesh commented Aug 18, 2020
View reviewed changes
sdk/iot/Azure.Iot.Hub.Service/tests/DevicesClientTests.cs
@@ -73,7 +73,7 @@ public async Task DevicesClient_IdentityLifecycle()
}
finally
{
await Cleanup(client, device);
await CleanupAsync(client, device).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to end with Async

timtay-microsoft
timtay-microsoft approved these changes Aug 18, 2020
View reviewed changes
Copy link
Member

@timtay-microsoft timtay-microsoft left a comment

Choose a reason for hiding this comment

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

Just a few notes that I'll leave to your judgement, but otherwise looks good

@vinagesh vinagesh merged commit a50e3cc into master Aug 18, 2020
@vinagesh vinagesh deleted the vinagesh/samples branch August 18, 2020 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timtay-microsoft timtay-microsoft timtay-microsoft approved these changes

@abhipsaMisra abhipsaMisra Awaiting requested review from abhipsaMisra

@azabbasi azabbasi Awaiting requested review from azabbasi

@barustum barustum Awaiting requested review from barustum

@bikamani bikamani Awaiting requested review from bikamani

@drwill-ms drwill-ms Awaiting requested review from drwill-ms

@prmathur-microsoft prmathur-microsoft Awaiting requested review from prmathur-microsoft

Assignees
No one assigned
Labels
IoT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vinagesh @drwill-ms @abhipsaMisra @timtay-microsoft