Azure / azure-iot-cli-extension

Azure IoT extension for Azure CLI
Other
83 stars 65 forks source link

Add Iot Central 2022-05-31 GA version support #525

Closed HangyiWang closed 2 years ago

HangyiWang commented 2 years ago

This PR was intended for adding iot central device group CRUD support. Now it's for iot central public API 2022-05-31 GA version support. (including device group CRUD and some other new commands)


This project has adopted the Microsoft Open Source Code of Conduct. For more information see the Code of Conduct FAQ or contact opencode@microsoft.com 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

Intent for Production

Basic expectations

Azure IoT CLI maintainers reserve the right to enforce any of the outlined expectations.

A PR is considered ready for review when all basic expectations have been met (or do not apply).

2022-06-30 17 16 45

2022-06-29 00 29 55

HangyiWang commented 2 years ago

Not sure why some checks failed. I don't have access to these details. 😒

digimaun commented 2 years ago

Not sure why some checks failed. I don't have access to these details. 😒

You should have access to see the pipeline runs now.

HangyiWang commented 2 years ago

Not sure why some checks failed. I don't have access to these details. 😒

You should have access to see the pipeline runs now.

There's an error for no defaults parameter in update command. But I found some other iot central update commands with default parameters. πŸ˜•

-  FAIL - HIGH severity: no_parameter_defaults_for_update_commands
    Parameter: iot central device-group update, `api_version` - Update commands should not have parameters with default values. 'api_version' in 'iot central device-group update' has a default value of '1.1-preview'
    Parameter: iot central device-group update, `central_dns_suffix` - Update commands should not have parameters with default values. 'central_dns_suffix' in 'iot central device-group update' has a default value of 'azureiotcentral.com'
HangyiWang commented 2 years ago

And there're 2 other errors not from iot central, but digital twins. I didn't change those parts. πŸ‘Œ

FAIL - MEDIUM severity: require_wait_command_if_no_wait
    Command-Group: `dt data-history connection create` - Group does not have a 'wait' command, yet 'dt data-history connection create adx' exposes '--no-wait'
    Command-Group: `dt endpoint create` - Group does not have a 'wait' command, yet 'dt endpoint create eventgrid' exposes '--no-wait'

-  FAIL - MEDIUM severity: parameter_should_not_end_in_resource_group
    Parameter: dt data-history connection create adx, `adx_resource_group` - A command should only have '--resource-group' as its resource group parameter. However options '--adx-resource-group' in command 'dt data-history connection create adx' end with 'resource-group' or similar.
    Parameter: dt data-history connection create adx, `eh_resource_group` - A command should only have '--resource-group' as its resource group parameter. However options '--eventhub-resource-group' in command 'dt data-history connection create adx' end with 'resource-group' or similar.
vilit1 commented 2 years ago

why not make some int tests too?

vilit1 commented 2 years ago

regarding https://github.com/Azure/azure-iot-cli-extension/pull/525#issuecomment-1151685055

I am going to update the linter exclusions for dt in my pr https://github.com/Azure/azure-iot-cli-extension/pull/515

HangyiWang commented 2 years ago

Adding 2022-05-31 version support for Iot central. UT and IT will be in next commit.

HangyiWang commented 2 years ago

regarding #525 (comment)

I am going to update the linter exclusions for dt in my pr #515

Thanks for updating. Somehow it didn't work in my PR so I updated the lint rules a little.

yang-hai-feng commented 2 years ago

Do we want to keep deprecated versions?

I think we should remove the deprecated versions from CLI support.

HangyiWang commented 2 years ago

Do we want to keep deprecated versions?

Will be removed in next PR since there're too many changes in this PR.

vilit1 commented 2 years ago

can you rerun the int tests after your changes? Try to run int tests whenever you change something

HangyiWang commented 2 years ago

can you rerun the int tests after your changes? Try to run int tests whenever you change something

I'll attach the screenshot very soon.

HangyiWang commented 2 years ago

Latest integration test result: 2022-06-30 17 16 45

HangyiWang commented 2 years ago

@digimaun Actually @vilit1 suggests me use az iot central device component twin, az iot central device component module twin and etc. to return twin in different component/module for customers. But @yang-hai-feng and I feel like it's unnecessary since we'll have 4 set of different twin command with 3 different operations in this way: get, update, replace. These 12 new commands could be simplified using az iot central device twin only. --component-name and --module-name is enough since component and module are the part of the device twin. Users won't have any confusion if they don't want to use --component-name or --module-name in command.

And I checked a command in iothub: iot hub digital-twin invoke-command. They are using --component-path to invoke command in device component but not using sth like iot hub digital-twin component invoke-command.

Any advice here?

yang-hai-feng commented 2 years ago

@digimaun Actually @vilit1 suggests me use az iot central device component twin, az iot central device component module twin and etc. to return twin in different component/module for customers. But @yang-hai-feng and I feel like it's unnecessary since we'll have 4 set of different twin command with 3 different operations in this way: get, update, replace. These 12 new commands could be simplified using az iot central device twin only. --component-name and --module-name is enough since component and module are the part of the device twin. Users won't have any confusion if they don't want to use --component-name or --module-name in command.

And I checked a command in iothub: iot hub digital-twin invoke-command. They are using --component-path to invoke command in device component but not using sth like iot hub digital-twin component invoke-command.

Any advice here?

@digimaun @lucadruda @vilit1, I would prefer to use --component-name and --module-name instead of module/component as separated group, module/component are more preferred within device model concept, that means from device template command you can show/update detailed definition for component or module, within device, more like a scope of properties|commands|telemetries, besides, using parameter could also simplify our backend implementations.

digimaun commented 2 years ago

@digimaun Actually @vilit1 suggests me use az iot central device component twin, az iot central device component module twin and etc. to return twin in different component/module for customers. But @yang-hai-feng and I feel like it's unnecessary since we'll have 4 set of different twin command with 3 different operations in this way: get, update, replace. These 12 new commands could be simplified using az iot central device twin only. --component-name and --module-name is enough since component and module are the part of the device twin. Users won't have any confusion if they don't want to use --component-name or --module-name in command. And I checked a command in iothub: iot hub digital-twin invoke-command. They are using --component-path to invoke command in device component but not using sth like iot hub digital-twin component invoke-command. Any advice here?

@digimaun @lucadruda @vilit1, I would prefer to use --component-name and --module-name instead of module/component as separated group, module/component are more preferred within device model concept, that means from device template command you can show/update detailed definition for component or module, within device, more like a scope of properties|commands|telemetries, besides, using parameter could also simplify our backend implementations.

The reason az iot hub digital-twin invoke-command looks the way it does is because in IoT Hub, the invoke digitaltwins command API operation does not differentiate between devices or modules, the top level resource is just referenced as a digital twin. It is also an outlier, considering other commands specific to device or module operations are generally separated as such (az iot hub module-identity create, az iot hub device-twin replace etc).

While elevating devEx is a top factor, of course the command groups / general command surface area has to make sense and click for an IoT Central user. Ideally someone who has a 100 level understanding of Central should be able to use and reason through IoT Central commands with minimal help. We are not going to block on this particular topic if you as the Central CLI product team are happy (and feel strongly) with this updated command mapping taking into account devEx and barrier to entry as outlined earlier. The Central CLI experience should bring users joy and fullfill whatever objective (automation or otherwise) a Central user/scenario needs.

yang-hai-feng commented 2 years ago

@digimaun Actually @vilit1 suggests me use az iot central device component twin, az iot central device component module twin and etc. to return twin in different component/module for customers. But @yang-hai-feng and I feel like it's unnecessary since we'll have 4 set of different twin command with 3 different operations in this way: get, update, replace. These 12 new commands could be simplified using az iot central device twin only. --component-name and --module-name is enough since component and module are the part of the device twin. Users won't have any confusion if they don't want to use --component-name or --module-name in command. And I checked a command in iothub: iot hub digital-twin invoke-command. They are using --component-path to invoke command in device component but not using sth like iot hub digital-twin component invoke-command. Any advice here?

@digimaun @lucadruda @vilit1, I would prefer to use --component-name and --module-name instead of module/component as separated group, module/component are more preferred within device model concept, that means from device template command you can show/update detailed definition for component or module, within device, more like a scope of properties|commands|telemetries, besides, using parameter could also simplify our backend implementations.

The reason az iot hub digital-twin invoke-command looks the way it does is because in IoT Hub, the invoke digitaltwins command API operation does not differentiate between devices or modules, the top level resource is just referenced as a digital twin. It is also an outlier, considering other commands specific to device or module operations are generally separated as such (az iot hub module-identity create, az iot hub device-twin replace etc).

While elevating devEx is a top factor, of course the command groups / general command surface area has to make sense and click for an IoT Central user. Ideally someone who has a 100 level understanding of Central should be able to use and reason through IoT Central commands with minimal help. We are not going to block on this particular topic if you as the Central CLI product team are happy (and feel strongly) with this updated command mapping taking into account devEx and barrier to entry as outlined earlier. The Central CLI experience should bring users joy and fullfill whatever objective (automation or otherwise) a Central user/scenario needs.

I see, we are using az iot central device edge module to operate on edge module command, the --module-name in az iot central device twin show --module-name, is concept from device model, different from edge module one.

HangyiWang commented 2 years ago

@digimaun Actually @vilit1 suggests me use az iot central device component twin, az iot central device component module twin and etc. to return twin in different component/module for customers. But @yang-hai-feng and I feel like it's unnecessary since we'll have 4 set of different twin command with 3 different operations in this way: get, update, replace. These 12 new commands could be simplified using az iot central device twin only. --component-name and --module-name is enough since component and module are the part of the device twin. Users won't have any confusion if they don't want to use --component-name or --module-name in command. And I checked a command in iothub: iot hub digital-twin invoke-command. They are using --component-path to invoke command in device component but not using sth like iot hub digital-twin component invoke-command. Any advice here?

@digimaun @lucadruda @vilit1, I would prefer to use --component-name and --module-name instead of module/component as separated group, module/component are more preferred within device model concept, that means from device template command you can show/update detailed definition for component or module, within device, more like a scope of properties|commands|telemetries, besides, using parameter could also simplify our backend implementations.

The reason az iot hub digital-twin invoke-command looks the way it does is because in IoT Hub, the invoke digitaltwins command API operation does not differentiate between devices or modules, the top level resource is just referenced as a digital twin. It is also an outlier, considering other commands specific to device or module operations are generally separated as such (az iot hub module-identity create, az iot hub device-twin replace etc).

While elevating devEx is a top factor, of course the command groups / general command surface area has to make sense and click for an IoT Central user. Ideally someone who has a 100 level understanding of Central should be able to use and reason through IoT Central commands with minimal help. We are not going to block on this particular topic if you as the Central CLI product team are happy (and feel strongly) with this updated command mapping taking into account devEx and barrier to entry as outlined earlier. The Central CLI experience should bring users joy and fullfill whatever objective (automation or otherwise) a Central user/scenario needs.

Thanks @digimaun . It won't take too much effort to add these new commands since I was mapping these APIs 1on1 to CLI commands by mistake. I just need to change few of them back.

The main point is that 'component' and 'module' is the part of the whole device twin. Each time users pass --component-name or --module-name, we'll get the whole instance twin data first and then extract the component/module data to them. If they don't specify any parameters, we'll return all twin. So the top-level resource is always the same. The difference is to return the whole data or filter part of the data based on the parameters.

digimaun commented 2 years ago

@digimaun Actually @vilit1 suggests me use az iot central device component twin, az iot central device component module twin and etc. to return twin in different component/module for customers. But @yang-hai-feng and I feel like it's unnecessary since we'll have 4 set of different twin command with 3 different operations in this way: get, update, replace. These 12 new commands could be simplified using az iot central device twin only. --component-name and --module-name is enough since component and module are the part of the device twin. Users won't have any confusion if they don't want to use --component-name or --module-name in command. And I checked a command in iothub: iot hub digital-twin invoke-command. They are using --component-path to invoke command in device component but not using sth like iot hub digital-twin component invoke-command. Any advice here?

@digimaun @lucadruda @vilit1, I would prefer to use --component-name and --module-name instead of module/component as separated group, module/component are more preferred within device model concept, that means from device template command you can show/update detailed definition for component or module, within device, more like a scope of properties|commands|telemetries, besides, using parameter could also simplify our backend implementations.

The reason az iot hub digital-twin invoke-command looks the way it does is because in IoT Hub, the invoke digitaltwins command API operation does not differentiate between devices or modules, the top level resource is just referenced as a digital twin. It is also an outlier, considering other commands specific to device or module operations are generally separated as such (az iot hub module-identity create, az iot hub device-twin replace etc). While elevating devEx is a top factor, of course the command groups / general command surface area has to make sense and click for an IoT Central user. Ideally someone who has a 100 level understanding of Central should be able to use and reason through IoT Central commands with minimal help. We are not going to block on this particular topic if you as the Central CLI product team are happy (and feel strongly) with this updated command mapping taking into account devEx and barrier to entry as outlined earlier. The Central CLI experience should bring users joy and fullfill whatever objective (automation or otherwise) a Central user/scenario needs.

Thanks @digimaun . It won't take too much effort to add these new commands since I was mapping these APIs 1on1 to CLI commands by mistake. I just need to change few of them back.

The main point is that 'component' and 'module' is the part of the whole device twin. Each time users pass --component-name or --module-name, we'll get the whole instance twin data first and then extract the component/module data to them. If they don't specify any parameters, we'll return all twin. So the top-level resource is always the same. The difference is to return the whole data or filter part of the data based on the parameters.

Understood - thanks for expanding the details here, in that case this pivot does make sense to me. Let's make sure there is enough docs / help guidance that this would be extra clear for users.

HangyiWang commented 2 years ago

@digimaun Actually @vilit1 suggests me use az iot central device component twin, az iot central device component module twin and etc. to return twin in different component/module for customers. But @yang-hai-feng and I feel like it's unnecessary since we'll have 4 set of different twin command with 3 different operations in this way: get, update, replace. These 12 new commands could be simplified using az iot central device twin only. --component-name and --module-name is enough since component and module are the part of the device twin. Users won't have any confusion if they don't want to use --component-name or --module-name in command. And I checked a command in iothub: iot hub digital-twin invoke-command. They are using --component-path to invoke command in device component but not using sth like iot hub digital-twin component invoke-command. Any advice here?

@digimaun @lucadruda @vilit1, I would prefer to use --component-name and --module-name instead of module/component as separated group, module/component are more preferred within device model concept, that means from device template command you can show/update detailed definition for component or module, within device, more like a scope of properties|commands|telemetries, besides, using parameter could also simplify our backend implementations.

The reason az iot hub digital-twin invoke-command looks the way it does is because in IoT Hub, the invoke digitaltwins command API operation does not differentiate between devices or modules, the top level resource is just referenced as a digital twin. It is also an outlier, considering other commands specific to device or module operations are generally separated as such (az iot hub module-identity create, az iot hub device-twin replace etc). While elevating devEx is a top factor, of course the command groups / general command surface area has to make sense and click for an IoT Central user. Ideally someone who has a 100 level understanding of Central should be able to use and reason through IoT Central commands with minimal help. We are not going to block on this particular topic if you as the Central CLI product team are happy (and feel strongly) with this updated command mapping taking into account devEx and barrier to entry as outlined earlier. The Central CLI experience should bring users joy and fullfill whatever objective (automation or otherwise) a Central user/scenario needs.

Thanks @digimaun . It won't take too much effort to add these new commands since I was mapping these APIs 1on1 to CLI commands by mistake. I just need to change few of them back. The main point is that 'component' and 'module' is the part of the whole device twin. Each time users pass --component-name or --module-name, we'll get the whole instance twin data first and then extract the component/module data to them. If they don't specify any parameters, we'll return all twin. So the top-level resource is always the same. The difference is to return the whole data or filter part of the data based on the parameters.

Understood - thanks for expanding the details here, in that case this pivot does make sense to me. Let's make sure there is enough docs / help guidance that this would be extra clear for users.

No problem. Will update this very soon.

HangyiWang commented 2 years ago

Latest E2E test results:

2022-07-06 14 24 26

2022-07-06 14 48 31

vilit1 commented 2 years ago

also did a test run https://dev.azure.com/azureiotdevxp/aziotcli/_build/results?buildId=6033&view=results (this is a branch on my fork made from this branch)

HangyiWang commented 2 years ago

This branch has been synced with base 'dev' branch.

vilit1 commented 2 years ago

newest test run https://dev.azure.com/azureiotdevxp/aziotcli/_build/results?buildId=6041&view=results