containerapp - Add integration tests for patch #6410
Conversation
|
Hi @snehapar9, |
ghost
requested a review
from 
ghost
added
the
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
harryli0108
left a comment
harryli0108
left a comment
There was a problem hiding this comment.
I think all looks good from my side. Meanwhile, commented a couple of questions.
| self.assertTrue(resp.ok) | ||
|
|
||
| # Re-run the 'az containerapp up' command with the location parameter if provided | ||
| if location: |
There was a problem hiding this comment.
I know this part is similar to the up command test case., but could you clarify the reason why we need to rerun the command with location argument instead of directly providing this as an argument?
There was a problem hiding this comment.
@harryli0108 just made some changes to re-use this method for containerapp up and patch commands.
| app = self.cmd(f"containerapp show -g {resource_group} -n {app_name}").get_output_in_json() | ||
| image = app["properties"]["template"]["containers"][0]["image"] | ||
| self.assertEquals(image,"mcr.microsoft.com/k8se/quickstart:latest") | ||
|
|
There was a problem hiding this comment.
I'm wondering if we could deploy 2 container apps (one with quick start image, one with an old patchable image) to make sure that the --show-all is working properly by adding assert to check how many results have been returned by the patcher list command.
There was a problem hiding this comment.
@harryli0108 In my opinion, it may be better to explicitly check for the image name than the count of container apps returned from patch list. The ordering may not be the same every time if we have multiple apps and would make it difficult to assert against the image name. I would think it may be okay to keep it simple with just a single container app. Thoughts?
cormacpayne
left a comment
cormacpayne
left a comment
There was a problem hiding this comment.
Minor comments, otherwise LGTM 
| repo = app["properties"]["template"]["containers"][0]["name"] | ||
|
|
||
| # Re-tag image to an older version to simulate patch list | ||
| old_tag = "run-dotnet-aspnet-7.0.1-cbl-mariner2.0" |
There was a problem hiding this comment.
Let's make old_tag an argument of this function to allow tests to be flexible with the older version they want to use
| @@ -0,0 +1,156 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Nope, we do not. Removed this line
| TEST_DIR = os.path.abspath(os.path.join(os.path.abspath(__file__), '..')) | ||
|
|
||
| class ContainerAppPatchTest(ScenarioTest): | ||
| @live_only() |
There was a problem hiding this comment.
May I ask why do you need to mark these new tests as @live_only()?
There was a problem hiding this comment.
@zhoxing-ms, these tests are marked @live_only() because they require docker to be running. Since CI does not have docker installed they would not work if they aren't run live.
5 participants
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally?For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.