-
Notifications
You must be signed in to change notification settings - Fork 195
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.
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
Correctly show function app resources in provision log #1503
Conversation
Function Apps and App Service apps use the same Azure resource types, so our display logic needs to query information about the provisioned resource to understand if a provisioned resource is an app service or a function app. Our existing logic to do this was broken when we moved away from using `az` to get information about a resource, since we were not passing an API version on our query. This caused our REST call to fail, and we just ignored the error (under the assumption it was a transient issue and we were okay displaying slightly wrong output for progress in this case). This change addresses the issue by forcing callers (who in theory understand what API version is valid for the resource they want to fetch information about) to provide an API verison and updates the one call site we have to pass a valid API version. Without this change, when deploying a template like `todo-nodejs-mongo-swa-func` you'd see this in the provision log: ``` (✓) Done: Web App: func-api-trxnle2vswr24 ``` Whereas with this change, we now correctly report that this is a function app: ``` (✓) Done: Function App: func-api-trxnle2vswr24 ``` Fixes Azure#1484
@weikanglim - This was a quick fix for the issue, I have a branch locally where I also added a Trying to think about the best way to write a test for this - I could imagine trying to write a unit test for the display logic and mock returning a deployment status object with both an app service and a function app and make sure the display logs the two lines correctly (after mocking the calls that will happen for |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash:
pwsh:
WindowsPowerShell install
MSI install
Standalone Binary
MSIContainer
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference (preview)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for fixing this!
@ellismg My first question would be if we cared enough about the accuracy of resource display. I could see it being acceptable, both as a product owner and as a product user, that these types of issues end up getting filed and fixed by the product team. If we want to invest effort here, I think we should be tactical about it, which means one of two things:
Otherwise, what broke here is the client-service contract. Specifically, the contract wasn't honored by the client. These are not things that can easily change, and for most cases, you would treat these as axiomatic. If they do change, you do want a test that catches the breakage. And thus, it can only be produced by verifying the actual client-server behavior, and not by mocked responses that would not catch a change in the service. Also, to note, the thing I'd care most about is that we make it impossible or near impossible to form an invalid client request. That seems to be taken care of with the change. |
Function Apps and App Service apps use the same Azure resource types, so our display logic needs to query information about the provisioned resource to understand if a provisioned resource is an app service or a function app.
Our existing logic to do this was broken when we moved away from using
az
to get information about a resource, since we were not passing an API version on our query. This caused our REST call to fail, and we just ignored the error (under the assumption it was a transient issue and we were okay displaying slightly wrong output for progress in this case).This change addresses the issue by forcing callers (who in theory understand what API version is valid for the resource they want to fetch information about) to provide an API verison and updates the one call site we have to pass a valid API version.
Without this change, when deploying a template like
todo-nodejs-mongo-swa-func
you'd see this in the provision log:Whereas with this change, we now correctly report that this is a function app:
Fixes #1484