Address GitHub Copilot review comments

- Add validation for aggregate function values (must be in AGGREGATE_FUNCTIONS)
- Add validation for duration-unit and granularity-unit (must be min/hr/day)
- Enforce entity-ids requirement for non-objectstorage services
- Validate that granularity and granularity-unit are provided together
- Add -h flag handling for help (in addition to --help)
- Make payload printing debug-only (only shown with --debug flag)
- Add @requires_jwe_token skip decorator for integration tests
- Add new test cases for validation improvements

Author: susharma-beep

linodecli/plugins/monitor-api.py

@@ -45,6 +45,9 @@ def get_auth_token():
 # Aggregate functions
 AGGREGATE_FUNCTIONS = ["sum", "avg", "max", "min", "count"]
 
+# Allowed time units
+ALLOWED_TIME_UNITS = ["min", "hr", "day"]
+
 
 @dataclass
 class MetricsConfig:
@@ -123,8 +126,32 @@ def parse_metrics(metrics: List[str]) -> List[dict]:
     for metric in metrics:
         if ":" in metric:
             metric_name, agg_func = metric.split(":", 1)
+            metric_name = metric_name.strip()
+            agg_func = agg_func.strip().lower()
+            if not metric_name:
+                print(
+                    f"Metric name is required for metric '{metric}'",
+                    file=sys.stderr,
+                )
+                print(
+                    f"Format: 'metric_name:function' where function is one of: "
+                    f"{', '.join(AGGREGATE_FUNCTIONS)}",
+                    file=sys.stderr,
+                )
+                sys.exit(ExitCodes.REQUEST_FAILED)
+            if agg_func not in AGGREGATE_FUNCTIONS:
+                print(
+                    f"Invalid aggregate function '{agg_func}' for metric '{metric}'",
+                    file=sys.stderr,
+                )
+                print(
+                    f"Aggregate function must be one of: "
+                    f"{', '.join(AGGREGATE_FUNCTIONS)}",
+                    file=sys.stderr,
+                )
+                sys.exit(ExitCodes.REQUEST_FAILED)
             parsed_metrics.append(
-                {"aggregate_function": agg_func, "name": metric_name.strip()}
+                {"aggregate_function": agg_func, "name": metric_name}
             )
         else:
             print(
@@ -206,7 +233,7 @@ def build_payload(config: MetricsConfig) -> dict:
     return payload
 
 
-def get_metrics(config: MetricsConfig):
+def get_metrics(config: MetricsConfig, debug: bool = False):
     """Get metrics for specified service entities"""
     payload = build_payload(config)
 
@@ -216,7 +243,12 @@ def get_metrics(config: MetricsConfig):
         )
     else:
         print(f"Fetching metrics for {config.service_name} (all entities)")
-    print(f"Request payload: {json.dumps(payload, indent=2)}")
+
+    if debug:
+        print(
+            f"Request payload: {json.dumps(payload, indent=2)}",
+            file=sys.stderr,
+        )
 
     try:
         status, response = make_api_request(
@@ -432,6 +464,18 @@ def validate_arguments(parsed):
         print("  --metrics: required", file=sys.stderr)
         return False
 
+    # Validate entity_ids requirement (only objectstorage allows querying all entities)
+    if not parsed.entity_ids and parsed.service.lower() != "objectstorage":
+        print(
+            f"--entity-ids is required for service '{parsed.service}'",
+            file=sys.stderr,
+        )
+        print(
+            "Only 'objectstorage' service allows querying all entities without --entity-ids",
+            file=sys.stderr,
+        )
+        return False
+
     # Validate time duration arguments
     has_relative = (
         parsed.duration is not None and parsed.duration_unit is not None
@@ -451,6 +495,42 @@ def validate_arguments(parsed):
         )
         return False
 
+    # Validate duration-unit if provided
+    if parsed.duration is not None and parsed.duration_unit is not None:
+        if parsed.duration_unit not in ALLOWED_TIME_UNITS:
+            print(
+                f"Invalid duration unit '{parsed.duration_unit}'",
+                file=sys.stderr,
+            )
+            print(
+                f"Allowed units: {', '.join(ALLOWED_TIME_UNITS)}",
+                file=sys.stderr,
+            )
+            return False
+
+    # Validate granularity arguments
+    if (parsed.granularity is not None) != (
+        parsed.granularity_unit is not None
+    ):
+        print(
+            "Both --granularity and --granularity-unit must be provided together",
+            file=sys.stderr,
+        )
+        return False
+
+    # Validate granularity-unit if provided
+    if parsed.granularity_unit is not None:
+        if parsed.granularity_unit not in ALLOWED_TIME_UNITS:
+            print(
+                f"Invalid granularity unit '{parsed.granularity_unit}'",
+                file=sys.stderr,
+            )
+            print(
+                f"Allowed units: {', '.join(ALLOWED_TIME_UNITS)}",
+                file=sys.stderr,
+            )
+            return False
+
     return True
 
 
@@ -473,7 +553,12 @@ def call(args, context=None):  # pylint: disable=unused-argument
     parsed, remaining_args = parser.parse_known_args(args)
 
     # Handle help cases
-    if not parsed.command or parsed.command == "help" or "--help" in args:
+    if (
+        not parsed.command
+        or parsed.command == "help"
+        or "--help" in args
+        or "-h" in args
+    ):
         print_help(parser)
         sys.exit(ExitCodes.SUCCESS)
 
@@ -527,4 +612,4 @@ def call(args, context=None):  # pylint: disable=unused-argument
         associated_entity_region=parsed.associated_entity_region,
     )
 
-    get_metrics(config)
+    get_metrics(config, debug=parsed.debug)

tests/integration/monitor/test_plugin_get_metrics.py

@@ -2,6 +2,8 @@
 Integration tests for the get_metrics plugin
 """
 
+import os
+
 import pytest
 
 from linodecli.exit_codes import ExitCodes
@@ -13,6 +15,12 @@
 # Base command for monitor-api plugin
 BASE_CMD = ["linode-cli", "monitor-api", "get-metrics"]
 
+# Skip decorator for tests that require JWE_TOKEN
+requires_jwe_token = pytest.mark.skipif(
+    not os.getenv("JWE_TOKEN"),
+    reason="JWE_TOKEN environment variable not set",
+)
+
 
 def test_missing_required_args():
     """Test error handling for missing required arguments"""
@@ -135,6 +143,7 @@ def test_conflicting_time_params():
 
 
 @pytest.mark.smoke
+@requires_jwe_token
 def test_objstorage_metrics_basic():
     """Test get_metrics with objectstorage service (with authentication)"""
     # Use objectstorage service which doesn't require entity-ids
@@ -157,6 +166,7 @@ def test_objstorage_metrics_basic():
     assert "Fetching metrics" in output or "data" in output.lower()
 
 
+@requires_jwe_token
 def test_obj_metrics_with_filters():
     """Test get_metrics with objectstorage service and filters"""
     output = exec_test_command(
@@ -179,6 +189,7 @@ def test_obj_metrics_with_filters():
     assert "Fetching metrics" in output or "data" in output.lower()
 
 
+@requires_jwe_token
 def test_absolute_time_metrics():
     """Test get_metrics with objectstorage service and absolute time range"""
     output = exec_test_command(
@@ -220,3 +231,83 @@ def test_malformed_filters():
         ],
         expected_code=ExitCodes.REQUEST_FAILED,
     )
+
+
+def test_invalid_aggregate_function_value():
+    """Test handling of invalid aggregate function values"""
+    exec_failing_test_command(
+        BASE_CMD
+        + [
+            "nodebalancer",
+            "--entity-ids",
+            "123",
+            "--metrics",
+            "cpu_usage:invalid_func",  # Invalid aggregate function
+            "--duration",
+            "15",
+            "--duration-unit",
+            "min",
+        ],
+        expected_code=ExitCodes.REQUEST_FAILED,
+    )
+
+
+def test_entity_ids_required_for_non_objectstorage():
+    """Test that entity-ids is required for non-objectstorage services"""
+    exec_failing_test_command(
+        BASE_CMD
+        + [
+            "dbaas",
+            "--metrics",
+            "cpu_usage:avg",
+            "--duration",
+            "15",
+            "--duration-unit",
+            "min",
+        ],
+        expected_code=ExitCodes.REQUEST_FAILED,
+    )
+
+
+def test_invalid_granularity_unit():
+    """Test handling of invalid granularity unit"""
+    exec_failing_test_command(
+        BASE_CMD
+        + [
+            "nodebalancer",
+            "--entity-ids",
+            "123",
+            "--metrics",
+            "cpu_usage:avg",
+            "--duration",
+            "15",
+            "--duration-unit",
+            "min",
+            "--granularity",
+            "5",
+            "--granularity-unit",
+            "invalid_unit",
+        ],
+        expected_code=ExitCodes.REQUEST_FAILED,
+    )
+
+
+def test_granularity_without_unit():
+    """Test that granularity requires granularity-unit"""
+    exec_failing_test_command(
+        BASE_CMD
+        + [
+            "nodebalancer",
+            "--entity-ids",
+            "123",
+            "--metrics",
+            "cpu_usage:avg",
+            "--duration",
+            "15",
+            "--duration-unit",
+            "min",
+            "--granularity",
+            "5",
+        ],
+        expected_code=ExitCodes.REQUEST_FAILED,
+    )