Addressing PR comments and issues after bug bash

Author: Himanshu49

src/load/azext_load/data_plane/load_trigger/custom.py

@@ -69,7 +69,7 @@ def create_trigger_schedule(
         response = client.create_or_update_trigger(trigger_id=trigger_id, body=trigger_body)
         logger.debug("Created trigger schedule: %s", response)
         logger.info("Creating trigger schedule completed")
-        return response
+        return response.as_dict()
     except Exception:
         logger.error("Error occurred while creating schedule trigger.")
         raise
@@ -120,20 +120,20 @@ def update_trigger_schedule(
         existing_trigger_schedule.recurrence
     )
     logger.debug("Recurrence object: %s", recurrence_body)
-    new_trigger_body = utils.get_schedule_trigger_body_for_update(
-        existing_trigger_schedule,
-        recurrence_body,
-        display_name,
-        description,
-        trigger_start_date_time,
-        test_ids,
+    new_trigger_body = models.ScheduleTestsTrigger(
+        test_ids=test_ids,
+        recurrence=recurrence_body,
+        start_date_time=trigger_start_date_time,
+        display_name=display_name,
+        description=description,
     )
+    new_trigger_body.state = existing_trigger_schedule.state
     logger.debug("Schedule trigger body to be sent for update: %s", new_trigger_body)
     try:
         response = client.create_or_update_trigger(trigger_id=trigger_id, body=new_trigger_body)
         logger.debug("Updated schedule trigger: %s", response)
         logger.info("Updating schedule trigger completed")
-        return response
+        return response.as_dict()
     except Exception:
         logger.error("Error occurred while updating schedule trigger.")
         raise
@@ -165,7 +165,7 @@ def get_trigger_schedule(
     client = get_admin_data_plane_client(cmd, load_test_resource, resource_group_name)
     response = client.get_trigger(trigger_id)
     logger.debug("Fetched schedule trigger: %s", response)
-    return response
+    return response.as_dict()
 
 
 def pause_trigger_schedule(
@@ -190,12 +190,12 @@ def pause_trigger_schedule(
         existing_trigger_schedule.state = enums.TriggerState.PAUSED
         response = client.create_or_update_trigger(trigger_id=trigger_id, body=existing_trigger_schedule)
         logger.debug("Paused schedule trigger: %s", response)
-        return response
+        return response.as_dict()
     if existing_trigger_schedule.state == enums.TriggerState.COMPLETED:
-        msg = "Schedule trigger with id: {} is already completed.".format(trigger_id)
+        msg = "Schedule trigger with id: {} is already completed. A completed schedule cannot be paused.".format(trigger_id)
         logger.error(msg)
         raise ValidationError(msg)
-    logger.warning("Schedule trigger is not active. It is in %s state.", existing_trigger_schedule.state.value)
+    logger.warning("Schedule trigger is not active. It is in %s state. Enable the schedule before performing pause action.", existing_trigger_schedule.state.value)
 
 
 def enable_trigger_schedule(
@@ -220,8 +220,8 @@ def enable_trigger_schedule(
         existing_trigger_schedule.state = enums.TriggerState.ACTIVE
         response = client.create_or_update_trigger(trigger_id=trigger_id, body=existing_trigger_schedule)
         logger.debug("Enabled schedule trigger: %s", response)
-        return response
-    msg = "Schedule trigger with id: {} is already completed. Cannot enable a completed schedule.".format(trigger_id)
+        return response.as_dict()
+    msg = "Schedule trigger with id: {} is already completed. A completed schedule cannot be enabled.".format(trigger_id)
     logger.debug(msg)
     raise ValidationError(msg)
 
@@ -240,6 +240,6 @@ def list_trigger_schedules(
     if test_ids:
         test_ids = ",".join(test_ids)
     logger.info("Schedule trigger states: %s", trigger_states)
-    response = client.list_trigger(test_ids=test_ids, states=trigger_states)
-    logger.debug("Fetched list of schedule triggers: %s", response)
-    return response
+    response_list = client.list_trigger(test_ids=test_ids, states=trigger_states)
+    logger.debug("Fetched list of schedule triggers: %s", response_list)
+    return [response.as_dict() for response in response_list]

src/load/azext_load/data_plane/load_trigger/utils.py

@@ -36,7 +36,7 @@ def get_recurrence_end_body(end_after_occurrence, end_after_date_time, existing_
     return existing_recurrence_end
 
 
-def handle_daily_recurrence(recurrence_interval, recurrence_end_body, **kwargs):
+def _handle_daily_recurrence(recurrence_interval, recurrence_end_body, **kwargs):
     if recurrence_interval is None:
         raise InvalidArgumentValueError("Recurrence interval is required for daily recurrence.")
     if any(kwargs.values()):
@@ -47,7 +47,7 @@ def handle_daily_recurrence(recurrence_interval, recurrence_end_body, **kwargs):
     )
 
 
-def handle_weekly_recurrence(recurrence_interval, recurrence_week_days, recurrence_end_body, **kwargs):
+def _handle_weekly_recurrence(recurrence_interval, recurrence_week_days, recurrence_end_body, **kwargs):
     if recurrence_interval is None:
         raise InvalidArgumentValueError("Recurrence interval is required for weekly recurrence.")
     if recurrence_week_days is None or len(recurrence_week_days) == 0:
@@ -61,7 +61,7 @@ def handle_weekly_recurrence(recurrence_interval, recurrence_week_days, recurren
     )
 
 
-def handle_hourly_recurrence(recurrence_interval, recurrence_end_body, **kwargs):
+def _handle_hourly_recurrence(recurrence_interval, recurrence_end_body, **kwargs):
     if recurrence_interval is None:
         raise InvalidArgumentValueError("Recurrence interval is required for hourly recurrence.")
     if any(kwargs.values()):
@@ -72,7 +72,7 @@ def handle_hourly_recurrence(recurrence_interval, recurrence_end_body, **kwargs)
     )
 
 
-def handle_monthly_by_dates_recurrence(recurrence_interval, recurrence_dates_in_month, recurrence_end_body, **kwargs):
+def _handle_monthly_by_dates_recurrence(recurrence_interval, recurrence_dates_in_month, recurrence_end_body, **kwargs):
     if recurrence_interval is None:
         raise InvalidArgumentValueError("Recurrence interval is required for monthly by dates recurrence.")
     if recurrence_dates_in_month is None or len(recurrence_dates_in_month) == 0:
@@ -86,7 +86,7 @@ def handle_monthly_by_dates_recurrence(recurrence_interval, recurrence_dates_in_
     )
 
 
-def handle_monthly_by_days_recurrence(recurrence_interval, recurrence_week_days, recurrence_index, recurrence_end_body, **kwargs):
+def _handle_monthly_by_days_recurrence(recurrence_interval, recurrence_week_days, recurrence_index, recurrence_end_body, **kwargs):
     if recurrence_interval is None:
         raise InvalidArgumentValueError("Recurrence interval is required for monthly by days recurrence.")
     if recurrence_week_days is None or len(recurrence_week_days) == 0:
@@ -103,7 +103,7 @@ def handle_monthly_by_days_recurrence(recurrence_interval, recurrence_week_days,
     )
 
 
-def handle_cron_recurrence(recurrence_cron_expression, recurrence_end_body, **kwargs):
+def _handle_cron_recurrence(recurrence_cron_expression, recurrence_end_body, **kwargs):
     if recurrence_cron_expression is None:
         raise InvalidArgumentValueError("Recurrence cron expression is required for cron recurrence.")
     if any(kwargs.values()):
@@ -115,12 +115,12 @@ def handle_cron_recurrence(recurrence_cron_expression, recurrence_end_body, **kw
 
 
 recurrence_handlers = {
-    enums.Frequency.DAILY: handle_daily_recurrence,
-    enums.Frequency.WEEKLY: handle_weekly_recurrence,
-    enums.Frequency.HOURLY: handle_hourly_recurrence,
-    enums.Frequency.MONTHLY_BY_DATES: handle_monthly_by_dates_recurrence,
-    enums.Frequency.MONTHLY_BY_DAYS: handle_monthly_by_days_recurrence,
-    enums.Frequency.CRON: handle_cron_recurrence,
+    enums.Frequency.DAILY: _handle_daily_recurrence,
+    enums.Frequency.WEEKLY: _handle_weekly_recurrence,
+    enums.Frequency.HOURLY: _handle_hourly_recurrence,
+    enums.Frequency.MONTHLY_BY_DATES: _handle_monthly_by_dates_recurrence,
+    enums.Frequency.MONTHLY_BY_DAYS: _handle_monthly_by_days_recurrence,
+    enums.Frequency.CRON: _handle_cron_recurrence,
 }
 
 
@@ -195,37 +195,3 @@ def get_recurrence_body_for_update(
         recurrence_week_days,
         recurrence_end_body
     )
-
-
-def get_schedule_trigger_body_for_update(
-    existing_trigger_schedule,
-    recurrence_body,
-    display_name,
-    description,
-    trigger_start_date_time,
-    test_ids,
-):
-    new_trigger_body = models.ScheduleTestsTrigger(
-        test_ids=test_ids,
-        recurrence=recurrence_body,
-        start_date_time=trigger_start_date_time,
-        display_name=display_name,
-        description=description,
-    )
-
-    if test_ids is None:
-        new_trigger_body.test_ids = existing_trigger_schedule.test_ids
-
-    if trigger_start_date_time is None:
-        new_trigger_body.start_date_time = existing_trigger_schedule.start_date_time
-
-    if display_name is None:
-        new_trigger_body.display_name = existing_trigger_schedule.display_name
-
-    if description is None:
-        new_trigger_body.description = existing_trigger_schedule.description
-
-    if recurrence_body is None:
-        new_trigger_body.recurrence = existing_trigger_schedule.recurrence
-
-    return new_trigger_body

src/load/azext_load/data_plane/utils/argtypes.py

@@ -482,7 +482,7 @@
 )
 
 recurrence_dates_in_month = CLIArgumentType(
-    options_list=["--recurrence-dates-in-month"],
+    options_list=["--recurrence-dates"],
     nargs="+",
     type=int,
     validator=validators.validate_recurrence_dates_in_month,

src/load/azext_load/data_plane/utils/validators.py

@@ -26,28 +26,27 @@
 
 logger = get_logger(__name__)
 
+def _validate_id(namespace, id_name):
+    """Validates a generic ID"""
+    id_value = getattr(namespace, id_name, None)
+    if id_value is None:
+        raise InvalidArgumentValueError(f"{id_name} is required.")
+    if not isinstance(id_value, str):
+        raise InvalidArgumentValueError(
+            f"Invalid {id_name} type: {type(id_value)}. Expected a string."
+        )
+    if not re.match("^[a-z0-9_-]*$", id_value):
+        raise InvalidArgumentValueError(f"Invalid {id_name} value.")
+
 
 def validate_test_id(namespace):
     """Validates test-id"""
-    if not isinstance(namespace.test_id, str):
-        raise InvalidArgumentValueError(
-            f"Invalid test-id type: {type(namespace.test_id)}"
-        )
-    if not re.match("^[a-z0-9_-]*$", namespace.test_id):
-        raise InvalidArgumentValueError("Invalid test-id value")
+    _validate_id(namespace, "test_id")
 
 
 def validate_test_run_id(namespace):
     """Validates test-run-id"""
-    if namespace.test_run_id is None:
-        namespace.test_run_id = utils.get_random_uuid()
-    if not isinstance(namespace.test_run_id, str):
-        raise InvalidArgumentValueError(
-            f"Invalid test-run-id type: {type(namespace.test_run_id)}"
-        )
-    if not re.match("^[a-z0-9_-]*$", namespace.test_run_id):
-        raise InvalidArgumentValueError("Invalid test-run-id value")
-
+    _validate_id(namespace, "test_run_id")
 
 def _validate_akv_url(string, url_type="secrets|certificates|keys|storage"):
     """Validates Azure Key Vault URL"""
@@ -552,36 +551,30 @@ def validate_engine_ref_ids_and_type(incoming_engine_ref_id_type, engine_ref_ids
 
 def validate_trigger_id(namespace):
     """Validates trigger-id"""
-    if not isinstance(namespace.trigger_id, str):
-        raise InvalidArgumentValueError(
-            f"Invalid trigger-id type: {type(namespace.trigger_id)}"
-        )
-    if not re.match("^[a-z0-9_-]*$", namespace.trigger_id):
-        raise InvalidArgumentValueError("Invalid trigger-id value")
+    _validate_id(namespace, "trigger_id")
 
 
 def validate_recurrence_dates_in_month(namespace):
     if namespace.recurrence_dates_in_month is None:
         return
     if not isinstance(namespace.recurrence_dates_in_month, list):
         raise InvalidArgumentValueError(
-            f"Invalid recurrence-dates-in-month type: {type(namespace.recurrence_dates_in_month)}. \
+            f"Invalid recurrence-dates type: {type(namespace.recurrence_dates_in_month)}. \
                 Expected list of integers"
         )
     for item in namespace.recurrence_dates_in_month:
         if not isinstance(item, int) or item < 1 or item > 31:
             raise InvalidArgumentValueError(
-                f"Invalid recurrence-dates-in-month item: {item}. Expected integer between 1 and 31"
+                f"Invalid recurrence-dates item: {item}. Expected integer between 1 and 31"
             )
 
 
 def validate_schedule_test_ids(namespace):
     if namespace.test_ids is None:
         return
-    if isinstance(namespace.test_ids, list) and len(namespace.test_ids) != 1:
-        raise InvalidArgumentValueError(
-            f"Invalid test-ids type: {type(namespace.test_ids)}. \
-                Expected list of test ids"
-        )
+    if not isinstance(namespace.test_ids, list):
+        raise InvalidArgumentValueError("Invalid test-ids type: {}. Expected list of test id.".format(type(namespace.test_ids)))
+    if len(namespace.test_ids) != 1:
+        raise InvalidArgumentValueError("Currently we only support one test ID per schedule.")
     if not re.match("^[a-z0-9_-]*$", namespace.test_ids[0]):
-        raise InvalidArgumentValueError("Invalid test-id value")
+        raise InvalidArgumentValueError("Invalid test-id value.")