[functionapp] add support for app insights #7890 #8081
Conversation
fea526a to
ec2cd92
Compare
| @@ -363,6 +363,13 @@ def load_arguments(self, _): | |||
| help="Geographic location where Function App will be hosted. Use 'functionapp list-consumption-locations' to view available locations.") | |||
| c.argument('runtime', help='The functions runtime stack.', arg_type=get_enum_type(set(LINUX_RUNTIMES).union(set(WINDOWS_RUNTIMES)))) | |||
| c.argument('os_type', arg_type=get_enum_type(OS_TYPES), help="Set the OS type for the app to be created.") | |||
| c.argument('create_app_insights', arg_type=get_three_state_flag(return_label=True), help="Creates a new App Insights and adds to the new Function app. Needs --app-insights-location.") | |||
There was a problem hiding this comment.
This is a bit too much to expose 5 more arguments for just one scenario support.
I would much prefer you just author a command of az appinsights create (you can put it inside the appservice module for the time being), and let functionapp create take an argument to point to an existing appinsights resource.
CLI community are asking for app insights support for a long while, why not start to make contributions here to help other users?
Once the command is in, we can further add a few more to make CRUD complete. For reference, check out #5543
There was a problem hiding this comment.
@yugangw-msft, that makes sense. If people have been asking for support, I don't mind making this into a global az appinsights. I might also be nice to confirm with the app insights team, just so I don't end up duplicating work.
\cc @ahmedelnably @ahmelsayed
There was a problem hiding this comment.
@ahmedelnably, if we have a global az appinsights create, would we still need to add support for users to create appinsights using az functionapp create, or could we just allow users to provide an exisiting appinsights (which they can create az appinsights create, and make the functionapp point to that? Just like we do with storage.
There was a problem hiding this comment.
@yugangw-msft, on offline discussion with my team as well as the AppInsights team, we decided that I should not make az appinsights to avoid ownership and maintenance issues. Appinsights team do have this work in their backlog, so they should be able to take care of that. Just wanted to post an update here.
I still need to update this PR to avoid introducing the new args.
70470d3 to
41ad145
Compare
|
The PR is updated. As per offline discussion, |
|
We have a new release today, can you update the history.rst to start with a new version? |
41ad145 to
68147a9
Compare
|
@yugangw-msft, I just updated it. |
|
@ankitkumarr update the version here as well https://github.com/Azure/azure-cli/blob/dev/src/command_modules/azure-cli-appservice/setup.py#L17 |
|
@panchagnula @ankitkumarr , updating setup.py is no longer needed :), thanks for the improvement made by @tjprescott recently. Travis, what is the right header we should put in the history.rst? |
|
@yugangw-msft they can simply append the HISTORY entry to the one at the top. During release, we can easily tell which entries need to be lifted into the next version bump. |
|
@tjprescott, do you mean I should not create a new version entry in the EDIT: |
68147a9 to
f6e3c3c
Compare
f6e3c3c to
af057fa
Compare
Currently, azure-cli does not support creation or management of app insights. Therefore, there was no simple way to delete the App insights directly created by my tests. Would this be OK or should I look into other ways of deleting the App Insights created by tests (maybe by deleting the resource group, or using appinsights-sdk)
/cc @ahmedelnably @ahmelsayed @panchagnula
This checklist is used to make sure that common guidelines for a pull request are followed.