Conversation
There was a problem hiding this comment.
Pull request overview
Adds new CLI capabilities to support alias updates and build “component” aggregation, along with accompanying tests and minor CLI plumbing updates.
Changes:
- Introduces
update aliascommand to point an alias at a build and wires theupdatecommand group into the main CLI. - Extends
record buildto accept repeated--component NAME=BUILD_OR_ALIASoptions and sendscomponentsin the build payload. - Updates and adds tests to cover
componentspayload behavior and alias update behavior (including fail-fast mode).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/commands/update/test_alias.py | Adds coverage for update alias success and error/fail-fast behavior |
| tests/commands/record/test_build.py | Updates expected build payloads to include components; adds new component-related tests |
| smart_tests/utils/commands.py | Adds Command.UPDATE_ALIAS enum value for tracking |
| smart_tests/commands/update/alias.py | Implements update alias command behavior |
| smart_tests/commands/update/init.py | Adds update command group and registers alias subcommand |
| smart_tests/commands/record/build.py | Adds --component option, validation, and includes components in build payload |
| smart_tests/args4p/typer/init.py | Changes Exit to extend BaseException (to avoid broad except Exception swallowing) |
| smart_tests/main.py | Registers update group in the top-level CLI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| click.echo(f"Alias '{alias_name}' now points to build '{build_name}'") | ||
| except Exception as e: |
There was a problem hiding this comment.
On request failures (e.g., non-2xx from raise_for_status()), this command only prints a warning/error but never calls tracking_client.send_error_event(...). Other commands (e.g., verify) report failures to tracking; consider doing the same here (ideally distinguishing HTTP status errors vs internal exceptions) so alias update failures are observable.
| click.echo(f"Alias '{alias_name}' now points to build '{build_name}'") | |
| except Exception as e: | |
| click.echo(f"Alias '{alias_name}' now points to build '{build_name}'") | |
| except HTTPError as http_err: | |
| status_code = http_err.response.status_code if getattr(http_err, "response", None) is not None else None | |
| tracking_client.send_error_event( | |
| "update_alias_http_error", | |
| { | |
| "alias_name": alias_name, | |
| "build_name": build_name, | |
| "status_code": status_code, | |
| "error": str(http_err), | |
| }, | |
| ) | |
| warn_and_exit_if_fail_fast_mode(f"Failed to update alias: {http_err}") | |
| except Exception as e: | |
| tracking_client.send_error_event( | |
| "update_alias_internal_error", | |
| { | |
| "alias_name": alias_name, | |
| "build_name": build_name, | |
| "error": str(e), | |
| }, | |
| ) |
| def compute_components(): | ||
| for c in components: | ||
| if not c.key: | ||
| click.echo("Component name must not be empty", err=True) | ||
| raise typer.Exit(1) | ||
| names_seen: set = set() | ||
| for c in components: | ||
| if c.key in names_seen: | ||
| click.echo(f"Duplicate component name: '{c.key}'", err=True) | ||
| raise typer.Exit(1) | ||
| names_seen.add(c.key) | ||
| return [{"name": c.key, "build": c.value} for c in components] | ||
|
|
There was a problem hiding this comment.
compute_components() validates that the component name (c.key) is non-empty, but it never validates the component build/alias (c.value). Because parse_key_value() allows empty values (e.g. name=), this can send an invalid payload ({"build": ""}) to the API. Please add a non-empty check for c.value (and emit a clear error) before constructing the payload.
| res = client.request("put", f"builds/aliases/{alias_name}", payload={"build": build_name}) | ||
| res.raise_for_status() | ||
| click.echo(f"Alias '{alias_name}' now points to build '{build_name}'") |
There was a problem hiding this comment.
alias_name is interpolated directly into the request path (builds/aliases/{alias_name}) without URL-encoding or validation. If it contains / (or encoded slashes like %2f), the client can hit an unintended endpoint. Consider rejecting these characters (similar to record build's --build validation) or URL-encoding the path segment.
| {"name": "auth", "build": "staging-auth-svc"}, | ||
| ], | ||
| }, payload) | ||
|
|
There was a problem hiding this comment.
This test is missing @responses.activate. Without it, the command will attempt a real HTTP GET to /state during client.is_fail_fast_mode() before it reaches the duplicate-component validation, which can make the test slow/flaky and pollute output. Please add @responses.activate (and any needed mocked responses) like the other CLI tests in this file.
| @responses.activate |
| from typing import Annotated | ||
|
|
||
| import click | ||
| from requests import HTTPError |
There was a problem hiding this comment.
HTTPError is imported but never used in this module. Please remove the unused import (or catch HTTPError explicitly if that was the intent) to keep the file clean and avoid confusion about the expected error handling.
| from requests import HTTPError |
… aggregation.