[AppService] functionapp create: Check if storage account is network restricted
#30605
Conversation
kamperiadis
requested review from
jsntcy,
zhoxing-ms and
yanzhudd
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @kamperiadis, |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| functionapp create | cmd functionapp create added parameter configure_networking_later |
|
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
requested review from
yonzhan,
kairu-ms,
AllyW,
necusjz,
evelyn-ys and
calvinhzy
microsoft-github-policy-service
bot
added
the
Network
kamperiadis
force-pushed
the
networkrestrictedstorage
branch
from
d4417f0 to
1f976d5
Compare
|
I notice that the assertion for tests fail if there are resources that we create on customers' behalf. Does it mean we can only run those tests in live mode instead of relying on recordings? @zhoxing-ms |
Why can't these resources be dynamically created in the tests? |
The intention is for the command to create those resources in behalf of the customer, as opposed to the customer (or tests) to create them separately. |
|
If the resource is dynamically created through CLI commands in the test code, the create request will also be recorded in the recording file, so it supports execution in replay mode |
|
@zhoxing-ms The resources that are internally created by the command (not in the test code but in the command itself), the tool has trouble determining that it is the same call. For instance, in the changes I made, I am creating a new file share resource (e.g. fileServices/default/shares/functionapp000002092f77bfe918) when the
|
Could you please show me the related code link? |
|
@zhoxing-ms This is where we create a file share when the customer runs |
|
azure-cli/src/azure-cli/azure/cli/command_modules/appservice/custom.py Lines 5156 to 5158 in 17efde4 I have encountered similar issues before, and I suspect it is due to the CI pipeline not meeting this if condition during execution the test. Therefore, this resulted in the failure to execute |
|
@zhoxing-ms I do not think that if condition is the problem. As you can see in the exception I included above, it cannot find a match in the file share created:
When the test was run again and was compared against the recording, there were two different file share names (functionapp000002092f77bfe918 and functionapp00000215a2c24cb746). However, if I run the test in live mode, there are no exceptions thrown. Should I just make the tests run in live mode only? |
kamperiadis
force-pushed
the
networkrestrictedstorage
branch
from
17efde4 to
c99c070
Compare
…ion explictly skipped
…tests in live mode
kamperiadis
force-pushed
the
networkrestrictedstorage
branch
from
c99c070 to
9156908
Compare
|
@kamperiadis Could you confirm that there are no other commands in the test to create file share? If not, is it possible that the file share was created twice during local testing, but only once in the CI pipeline? |
| @@ -5128,7 +5144,10 @@ def create_functionapp(cmd, resource_group_name, name, storage_account, plan=Non | |||
| if (plan_info is not None and is_plan_elastic_premium(cmd, plan_info)) or consumption_plan_location is not None: | |||
| site_config.app_settings.append(NameValuePair(name='WEBSITE_CONTENTAZUREFILECONNECTIONSTRING', | |||
| value=con_string)) | |||
| site_config.app_settings.append(NameValuePair(name='WEBSITE_CONTENTSHARE', value=_get_content_share_name(name))) | |||
| content_share_name = _get_content_share_name(name) | |||
There was a problem hiding this comment.
I think this function may cause the dismatch between resource name and the recording file.
azure-cli/src/azure-cli/azure/cli/command_modules/appservice/custom.py
Lines 5425 to 5430 in c84cd74
There was a problem hiding this comment.
Hi @yanzhudd We actually do intend to create a new content share resource with a new name on behalf of the customer. Does this mean we should just make the tests run live? Or what would you require for this PR to be merged?
There was a problem hiding this comment.
yes, you can mark the tests as live only, and please remove the related recording files as well
There was a problem hiding this comment.
Thank you @yanzhudd Those tests are already marked as live_only and there are no recording files for them.
|
Please let me know if this PR is ready to review/merge. |
|
@yanzhudd @zhoxing-ms The PR is ready to be merged. Thank you. |
Related command
az functionapp createDescription
We need to throw exceptions during the function app creation process if the storage account is network restricted
Testing Guide
History Notes
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.