Skip to content
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

[appservice-kube] az functionapp create: Make --storage-account optional for Kubernetes function apps #4678

Merged

Conversation

StrawnSC
Copy link
Contributor

@StrawnSC StrawnSC commented Apr 13, 2022


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

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@ghost ghost added the Auto-Assign Auto assign by bot label Apr 13, 2022
@ghost ghost requested review from evelyn-ys, calvinhzy and yonzhan April 13, 2022 18:08
@ghost ghost assigned evelyn-ys Apr 13, 2022
@ghost ghost added this to the Apr 2022 (2022-04-26) milestone Apr 13, 2022
@ghost ghost added the Storage label Apr 13, 2022
@ghost ghost requested a review from jiasli April 13, 2022 18:08
@ghost ghost assigned jiasli Apr 13, 2022
@ghost ghost added the Account label Apr 13, 2022
@ghost ghost requested review from zhoxing-ms and wangzelin007 April 13, 2022 18:09
@ghost ghost assigned zhoxing-ms Apr 13, 2022
@ghost ghost added the App Services label Apr 13, 2022
@ghost ghost requested review from kairu-ms and jsntcy April 13, 2022 18:09
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 13, 2022

appservice-kube

@panchagnula
Copy link
Contributor

FYI @ lpapudippu

@wangzelin007
Copy link
Member

@StrawnSC,
Please fix the integration test error.

@panchagnula
Copy link
Contributor

@StrawnSC added do not merge label for now, since there is some confusion, that feature team wants to test this with private whl, before releasing. you are on the group chat as well, lets merge this once we get conformation from them. Thanks!

@StrawnSC StrawnSC marked this pull request as draft April 15, 2022 20:43
@StrawnSC StrawnSC marked this pull request as ready for review June 1, 2022 21:14
@StrawnSC
Copy link
Contributor Author

@zhoxing-ms could we get this released soon as well? Thanks!

@@ -18,6 +18,7 @@ class AppserviceKubernetesScenarioTest(ScenarioTest):

# not lima-specific
class WebappBasicE2EKubeTest(ScenarioTest):
@live_only()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you mark this test as @live_only()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test passes locally (run live or as a recording), but fails in the CI with: vcr.errors.CannotOverwriteExistingCassetteException. I just pushed the recording for this test and removed the live_only and now the tests are failing

Copy link
Contributor

@zhoxing-ms zhoxing-ms Jun 23, 2022

Choose a reason for hiding this comment

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

ERROR: Can't overwrite existing cassette ('/home/vsts/work/1/s/src/appservice-kube/azext_appservice_kube/tests/latest/recordings/test_linux_webapp_quick_create_kube.yaml') in your current record mode ('once').
No match for the request (<Request (POST) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/clitest.rg000001/providers/Microsoft.Web/validate?api-version=2021-03-01>) was found.
Found 6 similar requests with 1 different matcher(s) :

This problem is caused by the fact that a rest request for validation is not recorded in the yaml file

Copy link
Contributor

@zhoxing-ms zhoxing-ms Jun 23, 2022

Choose a reason for hiding this comment

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

@StrawnSC I guess this may be because the code used in the local tests is not up to date.
Could you please pull the latest code from CLI main repo for main CLI code to the local, and then try to re-record the yaml file?

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'm pretty sure I was using the latest version of the main CLI's dev branch. I'll try again just to be sure

Copy link
Contributor Author

@StrawnSC StrawnSC Jun 29, 2022

Choose a reason for hiding this comment

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

@zhoxing-ms it's still failing in the CI even when I use the latest code for the main CLI (and rerun azdev setup)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This testing issue seems unrelated to this change: the change was to az functionapp create whereas the test that passes locally but fails in the CI is testing az webapp create. Also, we've been getting requests from the function apps team to release this for a while. Can I go ahead and mark the test as live_only for now so we can release this?

Copy link
Contributor

@zhoxing-ms zhoxing-ms Jun 30, 2022

Choose a reason for hiding this comment

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

@StrawnSC OK, if you are sure that this problem does not affect the feature of this PR, you can mark the test as live_only to avoid blocking the merging of PR. You can solve the test problems in the follow-up PR

@StrawnSC StrawnSC requested a review from zhoxing-ms July 1, 2022 16:46
@zhoxing-ms zhoxing-ms merged commit 4059920 into Azure:main Jul 2, 2022
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