Skip to content

Commit

Permalink
[Containerapp] az containerapp update: Fix issue for minReplicas in…
Browse files Browse the repository at this point in the history
… `--yaml` or `--min-replicas` is not set when the value is 0 (Azure#28163)
  • Loading branch information
Greedygre authored and MaxHorstmann committed Jan 19, 2024
1 parent 9a61a5f commit ad5c5ce
Show file tree
Hide file tree
Showing 13 changed files with 15,279 additions and 12,421 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -948,13 +948,13 @@ def _remove_readonly_attributes(containerapp_def):
del containerapp_def['properties'][unneeded_property]


# Remove null/None properties in a model since the PATCH API will delete those. Not needed once we move to the SDK
# Remove null/None/empty properties in a model since the PATCH API will delete those. Not needed once we move to the SDK
def clean_null_values(d):
if isinstance(d, dict):
return {
k: v
for k, v in ((k, clean_null_values(v)) for k, v in d.items())
if v or isinstance(v, list)
if (isinstance(v, dict) and len(v.items()) > 0) or (not isinstance(v, dict) and v is not None)
}
if isinstance(d, list):
return [v for v in map(clean_null_values, d) if v]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ def update_containerapp_logic(cmd,
e["value"] = ""

update_map = {}
update_map['scale'] = min_replicas or max_replicas or scale_rule_name
update_map['scale'] = min_replicas is not None or max_replicas is not None or scale_rule_name
update_map['container'] = image or container_name or set_env_vars is not None or remove_env_vars is not None or replace_env_vars is not None or remove_all_env_vars or cpu or memory or startup_command is not None or args is not None or secret_volume_mount is not None
update_map['ingress'] = ingress or target_port
update_map['registry'] = registry_server or registry_user or registry_pass
Expand Down Expand Up @@ -1565,6 +1565,9 @@ def start_containerappjob_execution_yaml(cmd, name, resource_group_name, file_na

containerappjobexec_def = _convert_object_from_snake_to_camel_case(_object_to_dict(containerappjobexec_def))

# Remove "additionalProperties" attributes that are introduced in the deserialization.
_remove_additional_attributes(containerappjobexec_def)

# Clean null values since this is an update
containerappjobexec_def = clean_null_values(containerappjobexec_def)

Expand Down

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
# --------------------------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for license information.
# --------------------------------------------------------------------------------------------
import unittest

from azure.cli.command_modules.containerapp._utils import clean_null_values


class UtilsTest(unittest.TestCase):
def test_clean_empty_values(self):
test_dict = {

}
result = clean_null_values(test_dict)
self.assertEqual({}, result)

test_dict["properties"] = {}

result_new = clean_null_values(test_dict)
result_old = old_clean_null_values(test_dict)

# test remove empty {}
expect_result = {}
self.assertEqual(expect_result, result_new)
self.assertEqual(result_new, result_old)

test_dict = {
"properties": {
"environmentId": None,
"test": {},
"configuration": {
"secrets": None,
"activeRevisionsMode": None,
"ingress": {
"fqdn": None,
"external": None,
"targetPort": None,
"exposedPort": None,
"transport": None,
"traffic": [
{
"revisionName": None,
"weight": None,
"latestRevision": None,
"label": None
}
],
"customDomains": None,
"allowInsecure": None,
"ipSecurityRestrictions": None,
"stickySessions": None,
"clientCertificateMode": None,
"corsPolicy": None
},
"registries": None,
"dapr": None,
"maxInactiveRevisions": None
},
},
}

# test remove empty {}
result_new = clean_null_values(test_dict)
result_old = old_clean_null_values(test_dict)

expect_result = {'properties': {'configuration': {'ingress': {'traffic': []}}}}
self.assertEqual(expect_result, result_new)
self.assertEqual(result_new, result_old)

test_dict = {
"properties": {
"environmentId": None,
"test": {},
"configuration": {
"secrets": None,
"activeRevisionsMode": "Single",
"ingress": {
"fqdn": None,
"external": None,
"targetPort": 80,
"exposedPort": None,
"transport": "Auto",
"traffic": [
{
"revisionName": None,
"weight": 100,
"latestRevision": None,
"label": None
}
],
"customDomains": None,
"allowInsecure": None,
"ipSecurityRestrictions": None,
"stickySessions": None,
"clientCertificateMode": None,
"corsPolicy": None
},
"registries": None,
"dapr": None,
"maxInactiveRevisions": None
},
},
}

result_new = clean_null_values(test_dict)
result_old = old_clean_null_values(test_dict)

expect_result = {
'properties': {
'configuration': {
'activeRevisionsMode': 'Single',
'ingress': {
'targetPort': 80,
'traffic': [
{
'weight': 100
}
],
'transport': 'Auto'
}
}
}
}
self.assertEqual(expect_result, result_new)
self.assertEqual(result_new, result_old)

test_dict = {
"properties": {
"environmentId": None,
"test": {
"secrets": "secretTest",
},
"configuration": {
"secrets": None,
"activeRevisionsMode": "Single",
"ingress": {
"fqdn": None,
"external": None,
"targetPort": 80,
"exposedPort": None,
"transport": "Auto",
"traffic": [
{
"revisionName": None,
"weight": 100,
"latestRevision": 0,
"label": None
}
],
"customDomains": None,
"allowInsecure": 0,
"ipSecurityRestrictions": None,
"stickySessions": None,
"clientCertificateMode": None,
"corsPolicy": None
},
"registries": None,
"dapr": None,
"maxInactiveRevisions": None
},
},
}

result_new = clean_null_values(test_dict)
result_old = old_clean_null_values(test_dict)
expect_result_for_new = {
'properties': {
"test": {
"secrets": "secretTest",
},
'configuration': {
'activeRevisionsMode': 'Single',
'ingress': {
'targetPort': 80,
'traffic': [
{
'weight': 100,
'latestRevision': 0,
}
],
'allowInsecure': 0,
'transport': 'Auto'
}
}
}
}
expect_result_for_old = {
'properties': {
"test": {
"secrets": "secretTest",
},
'configuration': {
'activeRevisionsMode': 'Single',
'ingress': {
'targetPort': 80,
'traffic': [
{
'weight': 100,
}
],
'transport': 'Auto'
}
}
}
}
self.assertEqual(expect_result_for_new, result_new)
self.assertEqual(expect_result_for_old, result_old)


# Remove null/None/empty properties in a model since the PATCH API will delete those. Not needed once we move to the SDK
# This old version will remove properties which value is 0
# The new version is to fix this bug and should not change other behaviors
def old_clean_null_values(d):
if isinstance(d, dict):
return {
k: v
for k, v in ((k, old_clean_null_values(v)) for k, v in d.items())
if v or isinstance(v, list)
}
if isinstance(d, list):
return [v for v in map(old_clean_null_values, d) if v]
return d
Original file line number Diff line number Diff line change
Expand Up @@ -1310,6 +1310,31 @@ def test_containerapp_scale_revision_copy(self, resource_group):
JMESPathCheck("name", env_name),
])

@AllowLargeResponse(8192)
@ResourceGroupPreparer(location="westeurope")
def test_containerapp_replica_commands(self, resource_group):
self.cmd(f'configure --defaults location={TEST_LOCATION}')

app_name = self.create_random_name(prefix='aca', length=24)
replica_count = 3

env = prepare_containerapp_env_for_app_e2e_tests(self)

self.cmd(f'containerapp create -g {resource_group} -n {app_name} --environment {env} --ingress external --target-port 80 --min-replicas {replica_count}', checks=[
JMESPathCheck("properties.provisioningState", "Succeeded"),
JMESPathCheck("properties.template.scale.minReplicas", 3)
]).get_output_in_json()

self.cmd(f'containerapp replica list -g {resource_group} -n {app_name}', checks=[
JMESPathCheck('length(@)', replica_count),
])
self.cmd(f'containerapp update -g {resource_group} -n {app_name} --min-replicas 0', checks=[
JMESPathCheck("properties.provisioningState", "Succeeded"),
JMESPathCheck("properties.template.scale.minReplicas", 0)
])

self.cmd(f'containerapp delete -g {resource_group} -n {app_name} --yes')

@AllowLargeResponse(8192)
@ResourceGroupPreparer(location="westeurope")
def test_containerapp_create_with_yaml(self, resource_group):
Expand Down Expand Up @@ -1437,7 +1462,7 @@ def test_containerapp_create_with_yaml(self, resource_group):
cpu: 0.5
memory: 1Gi
scale:
minReplicas: 1
minReplicas: 0
maxReplicas: 3
rules: []
"""
Expand All @@ -1454,7 +1479,7 @@ def test_containerapp_create_with_yaml(self, resource_group):
JMESPathCheck("properties.environmentId", containerapp_env["id"]),
JMESPathCheck("properties.template.revisionSuffix", "myrevision2"),
JMESPathCheck("properties.template.containers[0].name", "nginx"),
JMESPathCheck("properties.template.scale.minReplicas", 1),
JMESPathCheck("properties.template.scale.minReplicas", 0),
JMESPathCheck("properties.template.scale.maxReplicas", 3),
JMESPathCheck("properties.template.scale.rules", None)
])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,15 +202,15 @@ def test_container_acr_env_var(self, resource_group):
JMESPathCheck('length(properties.configuration.secrets)', 1),
])

# Update Container App with ACR
update_string = 'containerapp update -g {} -n {} --min-replicas 0 --max-replicas 1 --set-env-vars testenv=testing'.format(
# Update Container App with ACR, set --min-replicas 1
update_string = 'containerapp update -g {} -n {} --min-replicas 1 --max-replicas 1 --set-env-vars testenv=testing'.format(
resource_group, containerapp_name)
self.cmd(update_string, checks=[
JMESPathCheck('name', containerapp_name),
JMESPathCheck('properties.configuration.registries[0].server', registry_server),
JMESPathCheck('properties.configuration.registries[0].username', registry_username),
JMESPathCheck('length(properties.configuration.secrets)', 1),
JMESPathCheck('properties.template.scale.minReplicas', '0'),
JMESPathCheck('properties.template.scale.minReplicas', '1'),
JMESPathCheck('properties.template.scale.maxReplicas', '1'),
JMESPathCheck('length(properties.template.containers[0].env)', 1),
JMESPathCheck('properties.template.containers[0].env[0].name', "testenv"),
Expand All @@ -233,7 +233,7 @@ def test_container_acr_env_var(self, resource_group):
# Removing ACR password should fail since it is needed for ACR
self.cmd('containerapp secret remove -g {} -n {} --secret-names {}'.format(resource_group, containerapp_name, secret_name))

# Update Container App with ACR
# Update Container App with ACR, --min-replicas 0
update_string = 'containerapp update -g {} -n {} --min-replicas 0 --max-replicas 1 --replace-env-vars testenv=testing2'.format(
resource_group, containerapp_name)
self.cmd(update_string, checks=[
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -453,8 +453,8 @@ def test_containerappjob_eventtriggered_create_with_yaml(self, resource_group, l
replicaCompletionCount: 1
parallelism: 1
scale:
minExecutions: 0
maxExecutions: 10
minExecutions: 1
maxExecutions: 11
rules:
- name: github-runner-test
type: github-runner
Expand Down Expand Up @@ -527,8 +527,8 @@ def test_containerappjob_eventtriggered_create_with_yaml(self, resource_group, l
JMESPathCheck('properties.template.containers[0].resources.memory', "1Gi"),
JMESPathCheck('properties.configuration.eventTriggerConfig.replicaCompletionCount', 1),
JMESPathCheck('properties.configuration.eventTriggerConfig.parallelism', 1),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.minExecutions', 0),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.maxExecutions', 10),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.minExecutions', 1),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.maxExecutions', 11),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.rules[0].type', "github-runner"),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.rules[0].metadata.runnerScope', "repo"),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.rules[0].auth[0].secretRef',
Expand Down Expand Up @@ -556,7 +556,7 @@ def test_containerappjob_eventtriggered_create_with_yaml(self, resource_group, l
replicaCompletionCount: 2
parallelism: 2
scale:
minExecutions: 5
minExecutions: 0
maxExecutions: 95
rules:
- name: github-runner-testv2
Expand Down Expand Up @@ -588,7 +588,7 @@ def test_containerappjob_eventtriggered_create_with_yaml(self, resource_group, l
JMESPathCheck('properties.template.containers[0].image', "mcr.microsoft.com/k8se/quickstart-jobs:latest"),
JMESPathCheck('properties.configuration.eventTriggerConfig.replicaCompletionCount', 2),
JMESPathCheck('properties.configuration.eventTriggerConfig.parallelism', 2),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.minExecutions', 5),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.minExecutions', 0),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.maxExecutions', 95),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.rules[0].name', "github-runner-testv2"),
JMESPathCheck('properties.configuration.eventTriggerConfig.scale.rules[0].metadata.runnerScope', "repo"),
Expand Down

0 comments on commit ad5c5ce

Please sign in to comment.