Conversation
pmajali
requested review from
lgarber-akamai and
zliang-akamai
and removed request for
a team
yec-akamai
requested review from
a team,
Copilot,
jriddle-linode,
vshanthe and
yec-akamai
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yec-akamai
left a comment
yec-akamai
left a comment
There was a problem hiding this comment.
The command looks not quite right in regards to the url call. Can you make changes according to the comments?
susharma-beep
requested review from
ckulinsk
and removed request for
a team and
vshanthe
|
The cli command has been updated to below help commands |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
linodecli/plugins/monitor-api.py
Outdated
| parsed_metrics.append( | ||
| {"aggregate_function": agg_func, "name": metric_name.strip()} |
There was a problem hiding this comment.
parse_metrics accepts any aggregate function after : and sends it to the API without validation. Since AGGREGATE_FUNCTIONS is defined, validate agg_func (after .strip()/normalization) is one of the allowed values and fail fast with a clear error if not.
| parsed_metrics.append( | |
| {"aggregate_function": agg_func, "name": metric_name.strip()} | |
| 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} |
| # Validate time duration arguments | ||
| has_relative = ( | ||
| parsed.duration is not None and parsed.duration_unit is not None | ||
| ) | ||
| has_absolute = parsed.start_time is not None and parsed.end_time is not None | ||
|
|
||
| if not has_relative and not has_absolute: | ||
| print("Time duration required:", file=sys.stderr) | ||
| print(" Either: --duration and --duration-unit", file=sys.stderr) | ||
| print(" Or: --start-time and --end-time", file=sys.stderr) | ||
| return False | ||
|
|
||
| if has_relative and has_absolute: | ||
| print( | ||
| "Cannot specify both relative and absolute time duration", | ||
| file=sys.stderr, | ||
| ) | ||
| return False | ||
|
|
||
| return True |
There was a problem hiding this comment.
validate_arguments enforces presence of relative vs absolute time params, but it never validates the allowed values for --duration-unit / --granularity-unit (help text says min/hr/day). Add validation to reject unsupported units (and also fail if only one of --granularity/--granularity-unit is provided) so errors are caught client-side instead of relying on API failures.
| import pytest | ||
|
|
||
| from linodecli.exit_codes import ExitCodes | ||
| from tests.integration.helpers import ( | ||
| exec_failing_test_command, | ||
| exec_test_command, | ||
| ) | ||
|
|
||
| # Base command for monitor-api plugin | ||
| BASE_CMD = ["linode-cli", "monitor-api", "get-metrics"] | ||
|
|
There was a problem hiding this comment.
These integration tests call the real service and will fail hard if JWE_TOKEN isn’t set. Consider adding a pytest.mark.skipif(not os.getenv('JWE_TOKEN'), ...) guard (or a fixture) for the success-path tests so running pytest tests/integration without credentials doesn’t produce unrelated failures.
susharma-beep
force-pushed
the
dev
branch
2 times, most recently
from
d3cc043 to
87c3278
Compare
- Changed plugin base from 'linode-cli get_metrics' to 'linode-cli monitor-api' with 'get-metrics' as a subcommand - Refactored API_BASE_URL to be more generic, allowing API version specification - Updated all help text and examples to reflect new command structure - Updated unit and integration tests accordingly
…de#873) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 3.12.0 to 4.0.0. - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](docker/setup-buildx-action@8d2750c...4d04d5d) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-version: 4.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…elease notes workflow (linode#871)
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
- 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
yec-akamai
added
new-feature
pmajali
changed the title
📝 Description
A CLI plugin for the ACLP metric-read-api
✔️ How to Test
What are the steps to reproduce the issue or verify the changes?
linode-cli get_metrics dbaas --entity-ids 123 --duration 15 --duration-unit min --metrics cpu_usage:avg --filters 'node_type:in:primary,secondary'
the help command prints the list of sample commands. A JWE token is required to be set to make these calls.
details on generating JWE token https://techdocs.akamai.com/linode-api/reference/post-get-token
API details: https://techdocs.akamai.com/linode-api/reference/post-read-metric
How do I run the relevant unit/integration tests?
python -m pytest tests/unit/test_plugin_get_metrics.py -v
for integration set, set a JWE_TOKEN env var for service type objectstorage
python -m pytest tests/integration/monitor/test_plugin_get_metrics.py -v