Revision improvements (#113)

* Integer division to update existing weights. Validate revision names.

* Added revision label add ability to reuse existing label.

* Fixed style issues.

* Removed deprecated _round function in utils.

* Added revision label swap.

* Revision list only shows active revisions by default. Use flag --all to show inactive revisions as well.

* Updated history.

* Changed to use --labels instead of label1,label2. Added util to check if traffic payload less than 4096 bytes.

* Added label check in traffic set.

* Updated history.

* Removed 4096byte util.

* Added help for label swap, fixed styling.

* Added test for revision labels. Fixed help typo.

Co-authored-by: Haroon Feisal <haroonfeisal@microsoft.com>

scripts/ci/credscan/CredScanSuppressions.json

@@ -142,7 +142,8 @@
         "src\\containerapp\\azext_containerapp\\tests\\latest\\recordings\\test_containerapp_custom_domains_e2e.yaml",
         "src\\containerapp\\azext_containerapp\\tests\\latest\\cert.pfx",
         "src\\containerapp\\azext_containerapp\\tests\\latest\\test_containerapp_commands.py",
-        "src\\containerapp\\azext_containerapp\\tests\\latest\test_containerapp_env_commands.py"
+        "src\\containerapp\\azext_containerapp\\tests\\latest\\test_containerapp_env_commands.py",
+        "src\\containerapp\\azext_containerapp\\tests\\latest\\test_containerapp_revision_label_e2e.yaml"
       ],
       "_justification": "Dummy resources' keys left during testing Microsoft.App (required for log-analytics to create managedEnvironments)"
     }

src/containerapp/HISTORY.rst

@@ -5,6 +5,9 @@ Release History
 
 0.3.6
 ++++++
+* Added parameter --environment to 'az containerapp list'
+* Added 'az containerapp revision label swap' to swap traffic labels
+* BREAKING CHANGE: 'az containerapp revision list' now shows only active revisions by default, added flag --all to show all revisions
 
 
 0.3.5

src/containerapp/azext_containerapp/_help.py

@@ -289,6 +289,15 @@
           az containerapp revision label remove -n MyContainerapp -g MyResourceGroup --label myLabel
 """
 
+helps['containerapp revision label swap'] = """
+    type: command
+    short-summary: Swap a revision label between two revisions with associated traffic weights.
+    examples:
+    - name: Swap a revision label between two revisions..
+      text: |
+          az containerapp revision label swap -n MyContainerapp -g MyResourceGroup --labels myLabel1 myLabel2
+"""
+
 # Environment Commands
 helps['containerapp env'] = """
     type: group

src/containerapp/azext_containerapp/_params.py

@@ -200,6 +200,7 @@ def load_arguments(self, _):
 
     with self.argument_context('containerapp revision') as c:
         c.argument('revision_name', options_list=['--revision'], help='Name of the revision.')
+        c.argument('all', help='Boolean indicating whether to show inactive revisions.')
 
     with self.argument_context('containerapp revision copy') as c:
         c.argument('from_revision', help='Revision to copy from. Default: latest revision.')
@@ -209,6 +210,10 @@ def load_arguments(self, _):
         c.argument('name', id_part=None)
         c.argument('revision', help='Name of the revision.')
         c.argument('label', help='Name of the label.')
+        c.argument('yes', options_list=['--yes', '-y'], help='Do not prompt for confirmation.')
+
+    with self.argument_context('containerapp revision label') as c:
+        c.argument('labels', nargs='*', help='Labels to be swapped.')
 
     with self.argument_context('containerapp ingress') as c:
         c.argument('allow_insecure', help='Allow insecure connections for ingress traffic.')

src/containerapp/azext_containerapp/_utils.py

@@ -857,13 +857,27 @@ def _update_revision_weights(containerapp_def, list_weights):
     return old_weight_sum
 
 
+def _validate_revision_name(cmd, revision, resource_group_name, name):
+    if revision.lower() == "latest":
+        return
+    revision_def = None
+    try:
+        revision_def = ContainerAppClient.show_revision(cmd, resource_group_name, name, revision)
+    except:  # pylint: disable=bare-except
+        pass
+
+    if not revision_def:
+        raise ValidationError(f"Revision '{revision}' is not a valid revision name.")
+
+
 def _append_label_weights(containerapp_def, label_weights, revision_weights):
     if "traffic" not in containerapp_def["properties"]["configuration"]["ingress"]:
         containerapp_def["properties"]["configuration"]["ingress"]["traffic"] = []
 
     if not label_weights:
         return
 
+    bad_labels = []
     revision_weight_names = [w.split('=', 1)[0].lower() for w in revision_weights]  # this is to check if we already have that revision weight passed
     for new_weight in label_weights:
         key_val = new_weight.split('=', 1)
@@ -885,15 +899,17 @@ def _append_label_weights(containerapp_def, label_weights, revision_weights):
                 break
 
         if not is_existing:
-            raise ValidationError(f"No label {label} assigned to any traffic weight.")
+            bad_labels.append(label)
+
+    if len(bad_labels) > 0:
+        raise ValidationError(f"No labels '{', '.join(bad_labels)}' assigned to any traffic weight.")
 
 
 def _update_weights(containerapp_def, revision_weights, old_weight_sum):
 
     new_weight_sum = sum([int(w.split('=', 1)[1]) for w in revision_weights])
     revision_weight_names = [w.split('=', 1)[0].lower() for w in revision_weights]
     divisor = sum([int(w["weight"]) for w in containerapp_def["properties"]["configuration"]["ingress"]["traffic"]]) - new_weight_sum
-    round_up = True
     # if there is no change to be made, don't even try (also can't divide by zero)
     if divisor == 0:
         return
@@ -903,18 +919,17 @@ def _update_weights(containerapp_def, revision_weights, old_weight_sum):
     for existing_weight in containerapp_def["properties"]["configuration"]["ingress"]["traffic"]:
         if "latestRevision" in existing_weight and existing_weight["latestRevision"]:
             if "latest" not in revision_weight_names:
-                existing_weight["weight"], round_up = _round(scale_factor * existing_weight["weight"], round_up)
+                existing_weight["weight"] = round(scale_factor * existing_weight["weight"])
         elif "revisionName" in existing_weight and existing_weight["revisionName"].lower() not in revision_weight_names:
-            existing_weight["weight"], round_up = _round(scale_factor * existing_weight["weight"], round_up)
-
-
-# required because what if .5, .5? We need sum to be 100, so can't round up or down both times
-def _round(number, round_up):
-    import math
-    number = round(number, 2)  # required because we are dealing with floats
-    if round_up:
-        return math.ceil(number), not round_up
-    return math.floor(number), not round_up
+            existing_weight["weight"] = round(scale_factor * existing_weight["weight"])
+
+    total_sum = sum([int(w["weight"]) for w in containerapp_def["properties"]["configuration"]["ingress"]["traffic"]])
+    index = 0
+    while total_sum < 100:
+        weight = containerapp_def["properties"]["configuration"]["ingress"]["traffic"][index % len(containerapp_def["properties"]["configuration"]["ingress"]["traffic"])]
+        index += 1
+        total_sum += 1
+        weight["weight"] += 1
 
 
 def _validate_traffic_sum(revision_weights):

src/containerapp/azext_containerapp/commands.py

@@ -106,6 +106,7 @@ def load_command_table(self, _):
     with self.command_group('containerapp revision label') as g:
         g.custom_command('add', 'add_revision_label')
         g.custom_command('remove', 'remove_revision_label')
+        g.custom_command('swap', 'swap_revision_label')
 
     with self.command_group('containerapp ingress') as g:
         g.custom_command('enable', 'enable_ingress', exception_handler=ex_handler_factory())

src/containerapp/azext_containerapp/custom.py

@@ -62,7 +62,8 @@
                      _update_revision_env_secretrefs, _get_acr_cred, safe_get, await_github_action, repo_url_to_name,
                      validate_container_app_name, _update_weights, get_vnet_location, register_provider_if_needed,
                      generate_randomized_cert_name, _get_name, load_cert_file, check_cert_name_availability,
-                     validate_hostname, patch_new_custom_domain, get_custom_domains, get_resource_group)
+                     validate_hostname, patch_new_custom_domain, get_custom_domains, _validate_revision_name)
+
 
 from ._ssh_utils import (SSH_DEFAULT_ENCODING, WebSocketConnection, read_ssh, get_stdin_writer, SSH_CTRL_C_MSG,
                          SSH_BACKUP_ENCODING)
@@ -1254,9 +1255,12 @@ def delete_github_action(cmd, name, resource_group_name, token=None, login_with_
         handle_raw_exception(e)
 
 
-def list_revisions(cmd, name, resource_group_name):
+def list_revisions(cmd, name, resource_group_name, all=False):
     try:
-        return ContainerAppClient.list_revisions(cmd=cmd, resource_group_name=resource_group_name, name=name)
+        revision_list = ContainerAppClient.list_revisions(cmd=cmd, resource_group_name=resource_group_name, name=name)
+        if all:
+            return revision_list
+        return [r for r in revision_list if r["properties"]["active"]]
     except CLIError as e:
         handle_raw_exception(e)
 
@@ -1376,7 +1380,7 @@ def set_revision_mode(cmd, resource_group_name, name, mode, no_wait=False):
         handle_raw_exception(e)
 
 
-def add_revision_label(cmd, resource_group_name, revision, label, name=None, no_wait=False):
+def add_revision_label(cmd, resource_group_name, revision, label, name=None, no_wait=False, yes=False):
     _validate_subscription_registered(cmd, CONTAINER_APPS_RP)
 
     if not name:
@@ -1391,26 +1395,96 @@ def add_revision_label(cmd, resource_group_name, revision, label, name=None, no_
     if not containerapp_def:
         raise ResourceNotFoundError(f"The containerapp '{name}' does not exist in group '{resource_group_name}'")
 
-    if "ingress" not in containerapp_def['properties']['configuration'] and "traffic" not in containerapp_def['properties']['configuration']['ingress']:
-        raise ValidationError("Ingress and traffic weights are required to set labels.")
+    if "ingress" not in containerapp_def['properties']['configuration'] or "traffic" not in containerapp_def['properties']['configuration']['ingress']:
+        raise ValidationError("Ingress and traffic weights are required to add labels.")
 
-    traffic_weight = containerapp_def['properties']['configuration']['ingress']['traffic']
+    traffic_weight = containerapp_def['properties']['configuration']['ingress']['traffic'] if 'traffic' in containerapp_def['properties']['configuration']['ingress'] else {}
+
+    _validate_revision_name(cmd, revision, resource_group_name, name)
 
     label_added = False
     for weight in traffic_weight:
+        if "label" in weight and weight["label"].lower() == label.lower():
+            r_name = "latest" if "latestRevision" in weight and weight["latestRevision"] else weight["revisionName"]
+            if not yes and r_name.lower() != revision.lower():
+                msg = f"A weight with the label '{label}' already exists. Remove existing label '{label}' from '{r_name}' and add to '{revision}'?"
+                if not prompt_y_n(msg, default="n"):
+                    raise ArgumentUsageError(f"Usage Error: cannot specify existing label without agreeing to remove existing label '{label}' from '{r_name}' and add to '{revision}'.")
+            weight["label"] = None
+
         if "latestRevision" in weight:
             if revision.lower() == "latest" and weight["latestRevision"]:
                 label_added = True
                 weight["label"] = label
-                break
         else:
             if revision.lower() == weight["revisionName"].lower():
                 label_added = True
                 weight["label"] = label
-                break
 
     if not label_added:
-        raise ValidationError("Please specify a revision name with an associated traffic weight.")
+        containerapp_def["properties"]["configuration"]["ingress"]["traffic"].append({
+            "revisionName": revision if revision.lower() != "latest" else None,
+            "weight": 0,
+            "latestRevision": revision.lower() == "latest",
+            "label": label
+        })
+
+    containerapp_patch_def = {}
+    containerapp_patch_def['properties'] = {}
+    containerapp_patch_def['properties']['configuration'] = {}
+    containerapp_patch_def['properties']['configuration']['ingress'] = {}
+
+    containerapp_patch_def['properties']['configuration']['ingress']['traffic'] = traffic_weight
+
+    try:
+        r = ContainerAppClient.update(
+            cmd=cmd, resource_group_name=resource_group_name, name=name, container_app_envelope=containerapp_patch_def, no_wait=no_wait)
+        return r['properties']['configuration']['ingress']['traffic']
+    except Exception as e:
+        handle_raw_exception(e)
+
+
+def swap_revision_label(cmd, name, resource_group_name, labels, no_wait=False):
+    _validate_subscription_registered(cmd, CONTAINER_APPS_RP)
+
+    if not labels or len(labels) != 2:
+        raise ArgumentUsageError("Usage error: --labels requires two labels to be swapped.")
+
+    if labels[0] == labels[1]:
+        raise ArgumentUsageError("Label names to be swapped must be different.")
+
+    containerapp_def = None
+    try:
+        containerapp_def = ContainerAppClient.show(cmd=cmd, resource_group_name=resource_group_name, name=name)
+    except:
+        pass
+
+    if not containerapp_def:
+        raise ResourceNotFoundError(f"The containerapp '{name}' does not exist in group '{resource_group_name}'")
+
+    if "ingress" not in containerapp_def['properties']['configuration'] or "traffic" not in containerapp_def['properties']['configuration']['ingress']:
+        raise ValidationError("Ingress and traffic weights are required to swap labels.")
+
+    traffic_weight = containerapp_def['properties']['configuration']['ingress']['traffic'] if 'traffic' in containerapp_def['properties']['configuration']['ingress'] else {}
+
+    label1_found = False
+    label2_found = False
+    for weight in traffic_weight:
+        if "label" in weight:
+            if weight["label"].lower() == labels[0].lower():
+                if not label1_found:
+                    label1_found = True
+                    weight["label"] = labels[1]
+            elif weight["label"].lower() == labels[1].lower():
+                if not label2_found:
+                    label2_found = True
+                    weight["label"] = labels[0]
+    if not label1_found and not label2_found:
+        raise ArgumentUsageError(f"Could not find label '{labels[0]}' nor label '{labels[1]}' in traffic.")
+    if not label1_found:
+        raise ArgumentUsageError(f"Could not find label '{labels[0]}' in traffic.")
+    if not label2_found:
+        raise ArgumentUsageError(f"Could not find label '{labels[1]}' in traffic.")
 
     containerapp_patch_def = {}
     containerapp_patch_def['properties'] = {}
@@ -1439,8 +1513,8 @@ def remove_revision_label(cmd, resource_group_name, name, label, no_wait=False):
     if not containerapp_def:
         raise ResourceNotFoundError(f"The containerapp '{name}' does not exist in group '{resource_group_name}'")
 
-    if "ingress" not in containerapp_def['properties']['configuration'] and "traffic" not in containerapp_def['properties']['configuration']['ingress']:
-        raise ValidationError("Ingress and traffic weights are required to set labels.")
+    if "ingress" not in containerapp_def['properties']['configuration'] or "traffic" not in containerapp_def['properties']['configuration']['ingress']:
+        raise ValidationError("Ingress and traffic weights are required to remove labels.")
 
     traffic_weight = containerapp_def['properties']['configuration']['ingress']['traffic']
 
@@ -1565,6 +1639,9 @@ def set_ingress_traffic(cmd, name, resource_group_name, label_weights=None, revi
     if not containerapp_def:
         raise ResourceNotFoundError(f"The containerapp '{name}' does not exist in group '{resource_group_name}'")
 
+    if containerapp_def["properties"]["configuration"]["activeRevisionsMode"].lower() == "single":
+        raise ValidationError(f"Containerapp '{name}' is configured for single revision. Set revision mode to multiple in order to set ingress traffic. Try `az containerapp revision set-mode -h` for more details.")
+
     try:
         containerapp_def["properties"]["configuration"]["ingress"]
         containerapp_def["properties"]["configuration"]["ingress"]["traffic"]
@@ -1583,6 +1660,10 @@ def set_ingress_traffic(cmd, name, resource_group_name, label_weights=None, revi
     # update revision weights to containerapp, get the old weight sum
     old_weight_sum = _update_revision_weights(containerapp_def, revision_weights)
 
+    # validate revision names
+    for revision in revision_weights:
+        _validate_revision_name(cmd, revision.split('=')[0], resource_group_name, name)
+
     _update_weights(containerapp_def, revision_weights, old_weight_sum)
 
     containerapp_patch_def = {}