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

ACR changes + bug fixes #77

Merged
merged 5 commits into from
Apr 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
104 changes: 66 additions & 38 deletions src/containerapp/azext_containerapp/_up_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ def __init__(self, cmd, name: str, location: str, exists: bool = None):
self.check_exists()

def create(self):
if not self.name:
self.name = get_randomized_name(get_profile_username())
g = create_resource_group(self.cmd, self.name, self.location)
self.exists = True
return g
Expand All @@ -91,10 +89,12 @@ def check_exists(self) -> bool:

def create_if_needed(self):
if not self.check_exists():
logger.warning(f"Creating resoure group '{self.name}'")
if not self.name:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved this because I was getting logger warning: "Creating resource group None"

self.name = get_randomized_name(get_profile_username())
logger.warning(f"Creating resource group '{self.name}'")
self.create()
else:
logger.warning(f"Using resoure group '{self.name}'") # TODO use .info()
logger.warning(f"Using resource group '{self.name}'") # TODO use .info()


class Resource:
Expand Down Expand Up @@ -129,17 +129,6 @@ def check_exists(self):
self.exists = self.get() is not None
return self.exists

def create_if_needed(self, *args, **kwargs):
if not self.check_exists():
logger.warning(
f"Creating {type(self).__name__} '{self.name}' in resource group {self.resource_group.name}"
)
self.create(*args, **kwargs)
else:
logger.warning(
f"Using {type(self).__name__} '{self.name}' in resource group {self.resource_group.name}"
) # TODO use .info()


class ContainerAppEnvironment(Resource):
def __init__(
Expand All @@ -156,23 +145,25 @@ def __init__(
super().__init__(cmd, name, resource_group, exists)
if is_valid_resource_id(name):
self.name = parse_resource_id(name)["name"]
rg = parse_resource_id(name)["resource_group"]
if resource_group.name != rg:
self.resource_group = ResourceGroup(cmd, rg, location)
if "resource_group" in parse_resource_id(name):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bug fix part 1

rg = parse_resource_id(name)["resource_group"]
if resource_group.name != rg:
self.resource_group = ResourceGroup(cmd, rg, location)
self.location = _get_default_containerapps_location(cmd, location)
self.logs_key = logs_key
self.logs_customer_id = logs_customer_id

def set_name(self, name_or_rid):
if is_valid_resource_id(name_or_rid):
self.name = parse_resource_id(name_or_rid)["name"]
rg = parse_resource_id(name_or_rid)["resource_group"]
if self.resource_group.name != rg:
self.resource_group = ResourceGroup(
self.cmd,
rg,
_get_default_containerapps_location(self.cmd, self.location),
)
if "resource_group" in parse_resource_id(name_or_rid):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bug fix part 2

rg = parse_resource_id(name_or_rid)["resource_group"]
if self.resource_group.name != rg:
self.resource_group = ResourceGroup(
self.cmd,
rg,
_get_default_containerapps_location(self.cmd, self.location),
)
else:
self.name = name_or_rid

Expand All @@ -181,9 +172,20 @@ def _get(self):
self.cmd, self.resource_group.name, self.name
)

def create(self, app_name):
if self.name is None:
self.name = "{}-env".format(app_name).replace("_", "-")
def create_if_needed(self, app_name):
if not self.check_exists():
if self.name is None:
self.name = "{}-env".format(app_name).replace("_", "-")
logger.warning(
f"Creating {type(self).__name__} '{self.name}' in resource group {self.resource_group.name}"
)
self.create()
else:
logger.warning(
f"Using {type(self).__name__} '{self.name}' in resource group {self.resource_group.name}"
) # TODO use .info()

def create(self):
env = create_managed_environment(
self.cmd,
self.name,
Expand Down Expand Up @@ -275,12 +277,23 @@ def create(self, no_registry=False):
)

def create_acr_if_needed(self):
if self.should_create_acr:
logger.warning(
f"Creating Azure Container Registry {self.acr.name} in resource group "
f"{self.acr.resource_group.name}"
)
self.create_acr()
if self.acr:
self.acr.name, self.acr.resource_group.name, acr_found = find_existing_acr(self.cmd, self.env.resource_group.name, self.env.name, self.acr.name, self.acr.resource_group.name, self)
if self.should_create_acr:
logger.warning(
f"Creating Azure Container Registry {self.acr.name} in resource group "
f"{self.acr.resource_group.name}"
)
self.create_acr()
if acr_found:
self.registry_user, self.registry_pass, _ = _get_acr_cred(
self.cmd.cli_ctx, self.acr.name
)
self.registry_server = self.acr.name + ".azurecr.io"
logger.warning(
f"Using Azure Container Registry {self.acr.name} in resource group "
f"{self.acr.resource_group.name}"
)

def create_acr(self):
registry_rg = self.resource_group
Expand Down Expand Up @@ -608,11 +621,10 @@ def _get_registry_details(cmd, app: "ContainerApp"):
registry_rg = _get_acr_rg(app)
else:
registry_rg = app.resource_group.name
user = get_profile_username()
registry_name = app.name.replace("-", "").lower()
registry_name = app.env.name.replace("-", "").lower()
registry_name = (
(registry_name
+ str(hash((registry_rg, user, app.name)))
+ str(hash((app.env.resource_group.name, app.env.name)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change hash so it's more unique, as per @StrawnSC recommendation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Second part of the change needed here is to reconstruct the hash when we are trying to find the ACR from the env

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@StrawnSC I think the hash uses object of string instead of characters, so it is impossible to reconstruct the hash, you'll need to test this

.replace("-", "")
.replace(".", ""))[:10]
) # cap at 15 characters total
Expand Down Expand Up @@ -715,7 +727,7 @@ def up_output(app):
url = f"http://{url}"

logger.warning(
f"\nYour container app ({app.name}) has been created and deployed! Congrats! \n"
f"\nYour container app {app.name} has been created and deployed! Congrats! \n"
)
url and logger.warning(f"Browse to your container app at: {url} \n")
logger.warning(
Expand All @@ -724,3 +736,19 @@ def up_output(app):
logger.warning(
f"See full output using: az containerapp show -n {app.name} -g {app.resource_group.name} \n"
)


def find_existing_acr(cmd, resource_group_name, env_name, default_name, default_rg, app: "ContainerApp"):
from azure.cli.command_modules.acr.custom import acr_list as list_acr
from azure.cli.command_modules.acr._client_factory import cf_acr_registries
client = cf_acr_registries(cmd.cli_ctx)
acr_list = list_acr(client, resource_group_name)
acr_list = list(acr_list)
acr_list = [a for a in acr_list if env_name.lower().replace('-', '') in a.name.lower()]

if acr_list:
acr = acr_list[0]
app.should_create_acr = False
return acr.name, parse_resource_id(acr.id)["resource_group"], True
else:
return default_name, default_rg, False
2 changes: 1 addition & 1 deletion src/containerapp/azext_containerapp/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def load_command_table(self, _):
g.custom_command('update', 'update_containerapp', supports_no_wait=True, exception_handler=ex_handler_factory(), table_transformer=transform_containerapp_output)
g.custom_command('delete', 'delete_containerapp', supports_no_wait=True, confirmation=True, exception_handler=ex_handler_factory())
g.custom_command('exec', 'containerapp_ssh', validator=validate_ssh)
g.custom_command('up', 'containerapp_up', supports_no_wait=True, exception_handler=ex_handler_factory())
g.custom_command('up', 'containerapp_up', supports_no_wait=False, exception_handler=ex_handler_factory())
g.custom_command('browse', 'open_containerapp_in_browser')

with self.command_group('containerapp replica', is_preview=True) as g:
Expand Down
27 changes: 17 additions & 10 deletions src/containerapp/azext_containerapp/custom.py
Original file line number Diff line number Diff line change
Expand Up @@ -2016,7 +2016,7 @@ def containerapp_up(cmd,
from ._up_utils import (_validate_up_args, _reformat_image, _get_dockerfile_content, _get_ingress_and_target_port,
ResourceGroup, ContainerAppEnvironment, ContainerApp, _get_registry_from_app,
_get_registry_details, _create_github_action, _set_up_defaults, up_output, AzureContainerRegistry)

HELLOWORLD = "mcr.microsoft.com/azuredocs/containerapps-helloworld"
dockerfile = "Dockerfile" # for now the dockerfile name must be "Dockerfile" (until GH actions API is updated)

_validate_up_args(source, image, repo)
Expand All @@ -2025,14 +2025,14 @@ def containerapp_up(cmd,
image = _reformat_image(source, repo, image)
token = None if not repo else get_github_access_token(cmd, ["admin:repo_hook", "repo", "workflow"], token)

if image and "mcr.microsoft.com/azuredocs/containerapps-helloworld" in image.lower():
if image and HELLOWORLD in image.lower():
ingress = "external" if not ingress else ingress
target_port = 80 if not target_port else target_port

if image:
if ingress and not target_port:
target_port = 80
logger.warning("No ingress provided, defaulting to port 80.")
logger.warning("No ingress provided, defaulting to port 80. Try `az containerapp up --ingress %s --target-port <port>` to set a custom port.", ingress)

dockerfile_content = _get_dockerfile_content(repo, branch, token, source, context_path, dockerfile)
ingress, target_port = _get_ingress_and_target_port(ingress, target_port, dockerfile_content)
Expand All @@ -2047,12 +2047,13 @@ def containerapp_up(cmd,
if app.get()["properties"]["provisioningState"] == "InProgress":
raise ValidationError("Containerapp has an existing provisioning in progress. Please wait until provisioning has completed and rerun the command.")

resource_group.create_if_needed()
env.create_if_needed(name)

if source or repo:
_get_registry_from_app(app) # if the app exists, get the registry
_get_registry_details(cmd, app) # fetch ACR creds from arguments registry arguments

resource_group.create_if_needed()
env.create_if_needed(name)
app.create_acr_if_needed()

if source:
Expand Down Expand Up @@ -2085,6 +2086,7 @@ def containerapp_up_logic(cmd, resource_group_name, name, managed_env, image, en
if containerapp_def:
ca_exists = True

# When using repo, image is not passed, so we have to assign it a value (will be overwritten with gh-action)
if image is None:
image = "mcr.microsoft.com/azuredocs/containerapps-helloworld:latest"

Expand Down Expand Up @@ -2166,7 +2168,8 @@ def containerapp_up_logic(cmd, resource_group_name, name, managed_env, image, en
r["username"],
r["server"],
registry_pass,
update_existing_secret=True)
update_existing_secret=True,
disable_warnings=True)

# If not updating existing registry, add as new registry
if not updating_existing_registry:
Expand All @@ -2178,10 +2181,14 @@ def containerapp_up_logic(cmd, resource_group_name, name, managed_env, image, en
registry_user,
registry_server,
registry_pass,
update_existing_secret=True)
update_existing_secret=True,
disable_warnings=True)

registries_def.append(registry)

if ca_exists:
return ContainerAppClient.update(cmd, resource_group_name, name, containerapp_def)
return ContainerAppClient.create_or_update(cmd, resource_group_name, name, containerapp_def)
try:
if ca_exists:
return ContainerAppClient.update(cmd, resource_group_name, name, containerapp_def)
return ContainerAppClient.create_or_update(cmd, resource_group_name, name, containerapp_def)
except Exception as e:
handle_raw_exception(e)