Skip to content

Fix bug with az containerapp create #144

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.

Sign up for GitHub

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

Merged
StrawnSC merged 2 commits into from
Aug 19, 2022
Merged

Fix bug with az containerapp create #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7720e2d
fix bug with containerapp create
StrawnSC Aug 18, 2022
a285f20
add test case
StrawnSC Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/containerapp/HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Release History

0.3.10
++++++

* 'az containerapp create': Fix bug with --image caused by assuming a value for --registry-server

0.3.9
++++++
Expand Down
2 changes: 0 additions & 2 deletions src/containerapp/azext_containerapp/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,6 @@ def create_containerapp(cmd,
disable_warnings=False,
user_assigned=None,
registry_identity=None):
if image and "/" in image and not registry_server:
Copy link
Collaborator

@panchagnula panchagnula Aug 18, 2022

Choose a reason for hiding this comment

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

@StrawnSC I would assume this was added in first place for a reason - would there be any issues with removing this - that might need to be handled separately.

Copy link
Collaborator Author

@StrawnSC StrawnSC Aug 18, 2022

Choose a reason for hiding this comment

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

No, this was a last-minute change that we made in the last version because we thought it would make the user experience better. We had the faulty assumption that the user would always want to set a registry server value if they pass in an image

Copy link
Collaborator Author

@StrawnSC StrawnSC Aug 18, 2022

Choose a reason for hiding this comment

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

For context, all our testing of the MSI feature for az containerapp create involved using ACR registries like so:
az containerapp create --image sstrawn.azurecr.io/image:tag --registry-server sstrawn.azurecr.io
Based on that usage of the create command, it seemed like we could easily help the user our by trying to parse out the registry server from the --image value. For anonymous registries, the user doesn't need to add the --registry-server value, so this feature broke that use case

registry_server = image[:image.index("/")]
register_provider_if_needed(cmd, CONTAINER_APPS_RP)
validate_container_app_name(name)
validate_create(registry_identity, registry_pass, registry_user, registry_server, no_wait)
Expand Down
28 changes: 28 additions & 0 deletions src/containerapp/azext_containerapp/tests/latest/test_containerapp_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,34 @@ def test_containerapp_revision_label_e2e(self, resource_group):
self.assertEqual(len([w for w in traffic_weight if "label" in w]), 0)


class ContainerappAnonymousRegistryTests(ScenarioTest):
@AllowLargeResponse(8192)
@ResourceGroupPreparer(location="northeurope")
@live_only() # encounters 'CannotOverwriteExistingCassetteException' only when run from recording (passes when run live)
def test_containerapp_anonymous_registry(self, resource_group):
import requests

env = self.create_random_name(prefix='env', length=24)
app = self.create_random_name(prefix='aca', length=24)
image = "mcr.microsoft.com/azuredocs/containerapps-helloworld:latest"

self.cmd(f'containerapp env create -g {resource_group} -n {env}')

containerapp_env = self.cmd(f'containerapp env show -g {resource_group} -n {env}').get_output_in_json()

while containerapp_env["properties"]["provisioningState"].lower() == "waiting":
time.sleep(5)
containerapp_env = self.cmd(f'containerapp env show -g {resource_group} -n {env}').get_output_in_json()

self.cmd(f'containerapp create -g {resource_group} -n {app} --image {image} --ingress external --target-port 80 --environment {env}')

url = self.cmd(f'containerapp show -g {resource_group} -n {app}').get_output_in_json()["properties"]["configuration"]["ingress"]["fqdn"]
url = f"https://{url}"
resp = requests.get(url)
self.assertTrue(resp.ok)
self.assertEqual(resp.status_code, 200)


class ContainerappRegistryIdentityTests(ScenarioTest):
@AllowLargeResponse(8192)
@ResourceGroupPreparer(location="westeurope")
Expand Down