Add implementation for Devices APIs #12611

timtay-microsoft · 2020-06-08T19:58:18Z

This incorporates the recent design choices we've agreed on with regards to ETags.

Tests will be a separate PR

vinagesh · 2020-06-09T00:00:58Z

sdk/iot/Azure.Iot.Hub.Service/src/API Design.md

Create or update a device identity? Same comment for all APIs.

vinagesh · 2020-06-09T00:03:34Z

sdk/iot/Azure.Iot.Hub.Service/src/API Design.md

For this, can you check with SDK team about guidance on the type for input - ISet or IEnumerable or something else? I believe we only want to allow unique identities.

The annoying thing here is that a user can create two device identities with the same device id, and the set wouldn't catch that

But even still, that is a good question

Same comment for everything that has IEnumerable deviceIdentities as a parameter.

Leave it as it as but when you send an email about the IDictionary, just ask about this as well. That way we can represent our design choices well to the service team.

vinagesh · 2020-06-09T00:10:03Z

sdk/iot/Azure.Iot.Hub.Service/src/API Design.md

nit: The name feels a little inconsistent with the API that updates a single. However, twinPatches is also not great. We can probably call it - twin and twins?

Good catch. Maybe UpdateTwin(TwinData twinUpdate... and UpdateTwinsAsync(IEnumerable<TwinData> twinUpdates...

vinagesh · 2020-06-09T00:11:18Z

sdk/iot/Azure.Iot.Hub.Service/src/IfMatchPreconditionExtensions.cs

GetIfMatchHeaderValue?

azabbasi · 2020-06-09T16:04:03Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

if we use "identity" suffix in the summary, I think the parameter description also has to include the "identity" keyword

azabbasi · 2020-06-09T16:04:35Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

same here and through out this class

azabbasi · 2020-06-09T16:06:07Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

I think we should just be returning Tasks here, we don't need the "async" "await" pattern in the client. same goes for every other API in the document

azabbasi · 2020-06-09T16:09:54Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

I don't think you need to call ToList() here

Looks like you are right!

azabbasi · 2020-06-09T16:10:57Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

nit: break this line into multiple lines, same for the rest off them.

This is admittedly an ugly line of code, but where would you insert newlines?

We usually follow this pattern:

string.Equals(
    ExportImportDeviceStatus.Disabled.ToString(),
    x.Status?.ToString(),
    StringComparison.OrdinalIgnoreCase) ? ExportImportDeviceStatus.Disabled : ExportImportDeviceStatus.Enabled

tab for each parameter when there are many

Status = string.Equals(ExportImportDeviceStatus.Disabled.ToString(), x.Status?.ToString(), StringComparison.OrdinalIgnoreCase) ? ExportImportDeviceStatus.Disabled : ExportImportDeviceStatus.Enabled

azabbasi · 2020-06-09T16:11:29Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

use see href here

same for the rest of them.

azabbasi · 2020-06-09T16:12:45Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

I don't know what the wording for justification should be here, but probably not "pending"

I'll just get rid of this for now. This suppression is something we can come back to later

azabbasi · 2020-06-09T16:13:49Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

just device twin ? not sure ...

I don't follow. This is the get call for a device's twin. What else is it getting?

we have "Device Twin" in multiple places, the 's looks strange to me. it may just be me, you can ignore this :D

azabbasi · 2020-06-09T16:14:35Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

nit: just The new representation of the device twin

azabbasi · 2020-06-09T16:16:16Z

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

Should we change this type to be BulkRegistryOperationResponse? or even better, should we just remove the term Registry here? since it doesn't make sense anymore now that we have encapsulated the registry manager terminologies?

For Result vs Response, sure I like Response, too. That work belongs to the swagger edit effort, though. This is a generated type

I like removing the 'Registry' but maybe just post on the teams channel if everyone is onboard or has any concerns.

you would just change the type name in the custom code right?

I could change it in the custom code, sure

I'll add it to the modified swagger file as well, so we can hopefully not have custom code for this in the future

abhipsaMisra · 2020-06-09T17:07:23Z

sdk/iot/Azure.Iot.Hub.Service/src/API Design.md

What is the behavior if DeviceIdentity.Etag = null and precondition = IfMatchPrecondition.IfMatch?

Then a null ETag will be given to the PL, and the PL has logic to not send an If-Match header if the ETag is null. Effectively it is the same as just doing precondition = unconditional

abhipsaMisra · 2020-06-09T17:20:42Z

sdk/iot/Azure.Iot.Hub.Service/src/API Design.md

abhipsaMisra · 2020-06-09T17:23:29Z

sdk/iot/Azure.Iot.Hub.Service/src/API Design.md

nit: The description for precondition should be similar to what we have in other methods

abhipsaMisra · 2020-06-09T17:28:36Z

sdk/iot/Azure.Iot.Hub.Service/src/BulkIfMatchPrecondition.cs

Q - the matches in the description refers to the Etag match, correct? Why not say:
Perform this operation regardless of if the provided entity's ETag matches the service's ETag.?

Sure, but with resource instead of entity for consistency

abhipsaMisra · 2020-06-09T17:30:52Z

sdk/iot/Azure.Iot.Hub.Service/src/IfMatchPrecondition.cs

Should we also specify what happens on "delete" with "*" (when the resource doesn't exist).

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs

vinagesh · 2020-06-09T18:17:02Z

I have a general question - This is our apidesign.md. Should we do all the documentation right here and then copy during implementation or other way round? I know there is a lot of stuff we eventually added in ADT but we can all discuss and agree on the approach.

timtay-microsoft · 2020-06-09T18:22:03Z

I have a general question - This is our apidesign.md. Should we do all the documentation right here and then copy during implementation or other way round? I know there is a lot of stuff we eventually added in ADT but we can all discuss and agree on the approach.

In theory, the API design doc wasn't supposed to change after we all signed off. But in practice we keep coming up with small tweaks. I'd like to stop porting these changes to the API doc, but we can't delete that doc yet since some APIs are still under construction. So for now I just try to keep the doc consistent with the implementation

vinagesh · 2020-06-09T20:40:58Z

@timtay-microsoft - In that case you might want to check on adding all other tags too. Like <returns>, <exception> etc... Only <example> would come later.

* feat(hub): Creating a new feature branch with swagger and generated files * fix(doc): Fix markdown for API design doc (#11690) * swagger(iothub): Adding overrides for type names (#12026) * fix(tests): Fix project reference for the test framework (#12053) * fix(hub): Fix property accessibility issue (#12055) * Fix API categories for iothub service client (#12087) * swagger(iothub): Swagger comment changes (#12149) * fix(iot): Regenerate iothub PL after rebase from master * refactor(iot): Remove unnecessary custom code This class only existed to make the DigitalTwinClient internal, but now all generated clients are internal by default * Swagger changes for Iot Hub (#12218) * Revert swagger back to what is currently deployed This swagger should never be hand edited. We can update it only when service accepts the changes * Add composite swagger file with all suggested changes for service to make read only, required params, and comment refactors. OperationId changes will go in here, too * Regenerate PL with the currently deployed swagger * Update models to rename CloudToDeviceMethod and CloudToDeviceMethodResult (#12240) * Modules API design (#12188) * Add IoTHub Devices subclient APIs * Swagger changes for Client grouping (#12245) * Add suggested type name changes to iothub swagger (#12296) * Service Client CL and client grouping (#12323) * Small API design comments fix * feat(autorest): Generated clients from autorest after sync with master * Add implementation for Devices APIs (#12611) * (feat): Implement Modules client (#12673) * feat(tests): Add test infrastructure and setup.ps1 for local setup (#12719) * Add test infrastructure and setup * Add common files, remove specific sub (#12722) * fix(swagger): Fix IotHub swagger descriptions (#12695) * fix(pipeline): Update setup script to call test-resources ARM template directly (#12775) * feat(samples): Samples project skeleton (#12787) * IoT hub service client authentication via connection string (#12731) * feat(e2e-tests): Add initial setup for E2E tests * feat(iot-service): Add authentication via connection string * fix(iot-service): Fix merge conflict in infrastructure setup file (#12803) Co-authored-by: Abhipsa Misra <[email protected]> * feat(tests): Changes to fix tests and make sure we can run them successfully. (#12819) * Start recording tests and add intial Session recording (#12827) * feat(samples): Initial CREATE/DELETE sample for ModuleI (#12850) * feat(samples): Finish Modules samples (#12989) * feat(e2e): Devices E2E tests (#12997) * Update the logic for ETags and preconditions (#13046) * Fix the CI and test pipelines. (#13091) Co-authored-by: abhipds <[email protected]> Co-authored-by: Abhipsa Misra <[email protected]> Co-authored-by: vinagesh <[email protected]> Co-authored-by: timtay-microsoft <[email protected]> Co-authored-by: bikamani <[email protected]> Co-authored-by: Sindhu Nagesh <[email protected]> Co-authored-by: Abhipsa Misra <[email protected]>

timtay-microsoft requested a review from AlexGhiondea as a code owner June 8, 2020 19:58

timtay-microsoft added the IoT label Jun 8, 2020

vinagesh reviewed Jun 9, 2020

View reviewed changes

vinagesh approved these changes Jun 9, 2020

View reviewed changes

azabbasi reviewed Jun 9, 2020

View reviewed changes

azabbasi added the IoT Managed SDK label Jun 9, 2020

abhipsaMisra reviewed Jun 9, 2020

View reviewed changes

sdk/iot/Azure.Iot.Hub.Service/src/API Design.md Outdated

Copy link

Member

abhipsaMisra Jun 9, 2020

Choose a reason for hiding this comment

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

👍

timtay-microsoft reacted with thumbs up emoji

abhipsaMisra reviewed Jun 9, 2020

View reviewed changes

sdk/iot/Azure.Iot.Hub.Service/src/DevicesClient.cs Outdated Show resolved Hide resolved

vinagesh force-pushed the feature/IoT-Hub branch from 6698af4 to 87676d6 Compare June 9, 2020 18:29

timtay-microsoft force-pushed the feature/iot/timtay/devices branch from d39cbfa to 2739030 Compare June 9, 2020 18:32

Add implementation for Devices APIs

60abecc

timtay-microsoft force-pushed the feature/iot/timtay/devices branch from 2739030 to 60abecc Compare June 9, 2020 18:34

timtay-microsoft added 3 commits June 9, 2020 12:35

cr comments

f2fcfcb

cr comments

7010e22

cr comments

64ebb6b

Cr comments

faed9be

azabbasi approved these changes Jun 9, 2020

View reviewed changes

timtay-microsoft merged commit 7a82b0d into feature/IoT-Hub Jun 9, 2020

timtay-microsoft deleted the feature/iot/timtay/devices branch June 9, 2020 21:43

prmathur-microsoft pushed a commit that referenced this pull request Jul 7, 2020

Add implementation for Devices APIs (#12611)

b8ccf61

prmathur-microsoft pushed a commit that referenced this pull request Jul 8, 2020

Add implementation for Devices APIs (#12611)

8f192cc

Add implementation for Devices APIs #12611

Add implementation for Devices APIs #12611

Uh oh!

Conversation

timtay-microsoft commented Jun 8, 2020

Uh oh!

vinagesh Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinagesh Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timtay-microsoft Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinagesh Jun 9, 2020 •

edited

Loading

vinagesh Jun 9, 2020 •

edited

Loading

timtay-microsoft Jun 9, 2020 •

edited

Loading

abhipsaMisra Jun 9, 2020 •

edited

Loading

vinagesh commented Jun 9, 2020 •

edited

Loading