Skip to content

Conversation

@r-delgadillo
Copy link
Contributor

@r-delgadillo r-delgadillo commented Mar 20, 2020

History Notes

[IoT Central] Deprecate az iotcentral
[IoT Central] Add az iot central command module


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

@r-delgadillo r-delgadillo changed the title [Iot Central] (BREAKING CHANGE:) (az iotcentral) Deprecate 'az iotcentral' and being using 'az iot central' [Iot Central] BREAKING CHANGE: az iotcentral Deprecate 'az iotcentral' and being using 'az iot central' Mar 20, 2020
@r-delgadillo r-delgadillo changed the title [Iot Central] BREAKING CHANGE: az iotcentral Deprecate 'az iotcentral' and being using 'az iot central' [Iot Central] BREAKING CHANGE: az iotcentral deprecated Mar 20, 2020
@yungezz yungezz added this to the S167 milestone Mar 20, 2020
@yungezz
Copy link
Member

yungezz commented Mar 20, 2020

HI @fengzhou-msft could you pls help to review the PR? thanks.

@yungezz yungezz requested a review from fengzhou-msft March 20, 2020 08:07
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 20, 2020

iotcentral

@fengzhou-msft
Copy link
Member

Please run azdev style iot to fix the style issue and run azdev test iot --live (add --discover for first run) to run live tests and add recording files.

@r-delgadillo
Copy link
Contributor Author

r-delgadillo commented Mar 24, 2020

@fengzhou-msft During this experience, I had recommended to my team that I feel it's odd that we are putting control plane operations at a 4th level. For example, az iot central app create instead of something like az iot central create or az iot app create. Do you have any recommendations or guidance on which would provide a more develop friendly experience to our customers?

I read the general guidance docs but we'd like to get some insights from a member of the Azure CLI team.

@r-delgadillo
Copy link
Contributor Author

r-delgadillo commented Mar 25, 2020

Please run azdev style iot to fix the style issue and run azdev test iot --live (add --discover for first run) to run live tests and add recording files.

@fengzhou-msft I ran azdev style and test locally and pushed the changes but "checks" are still failing for test failures. I ran azdev test iot --live and they all passed when I ran them. DO you know what might be causing them to fail on the PR?

@fengzhou-msft
Copy link
Member

Please run azdev style iot to fix the style issue and run azdev test iot --live (add --discover for first run) to run live tests and add recording files.

@fengzhou-msft I ran azdev style and test locally and pushed the changes but "checks" are still failing for test failures. I ran azdev test iot --live and they all passed when I ran them. DO you know what might be causing them to fail on the PR?

Can you check if you commited the latest recording files after your successful live run?

@r-delgadillo r-delgadillo changed the title [DO NOT MERGE] [Iot Central] az iotcentral deprecated [Iot Central] az iotcentral deprecated Apr 13, 2020
@r-delgadillo
Copy link
Contributor Author

@fengzhou-msft Can we get this merged in. Thank you

@r-delgadillo
Copy link
Contributor Author

May I know why there are some deletions in src/azure-cli/azure/cli/command_modules/iot/tests/latest/recordings/test_iot_hub.yaml?

I don't believe there's any deletion. I ran the tests according to the documentation with azdev test iot --discover --live and all the tests passed.

@r-delgadillo
Copy link
Contributor Author

@Juliehzl @haroldrandom Can you please take a look at the PR and merge if it's ok. Thank you

help='Pricing tier for IoT Central applications. Default value is ST2.')
c.argument('subdomain',
help='Subdomain for the IoT Central URL. Each application must have a unique subdomain.')
c.argument('template',
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there allowed values for template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is. We have an API that allows customers to see the available options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not want to hardcode these values in the Azure CLI

Copy link
Contributor

Choose a reason for hiding this comment

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

In this way can we put the API in help message? Because for me (a new user), I totally don't know which value is available for template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can see how that can be confusing. We have an item in our backlog to rewrite the documentation for Swagger and Az CLI. I added some details about the issue of not seeing available templates so we can address it when we have the capacity.

g.custom_show_command('show', 'iot_central_app_get')
g.generic_update_command('update', getter_name='iot_central_app_get',
setter_name='iot_central_app_update', command_type=update_custom_util)
g.custom_command('delete', 'iot_central_app_delete')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add confirmation=True for delete command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted this change because this will break existing scripts customers are using. This will require them to pass in --yes in order for it proceeds with existing functionality we had before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, we really don't want to introduce any new functionality in this PR we just want to migrate existing command module code from iotcentral to iot

Copy link
Contributor

@Juliehzl Juliehzl Apr 16, 2020

Choose a reason for hiding this comment

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

Have you add deprecation information for original commands? And because you already give users breaking change, and they need to change the scripts anyway, it should be fine to add one more -y in their scripts.

Please feel free to let me know your concern.

Copy link
Contributor Author

@r-delgadillo r-delgadillo Apr 16, 2020

Choose a reason for hiding this comment

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

Yes, the deprecation information for az iotcentral command module is in ~/command_modules/iotcentral/commands.py in this PR. That makes sense about breaking them either way, I added it back thanks.

@Juliehzl Juliehzl merged commit 0b3b6bb into Azure:dev Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants