From b371bef0f8df1d38867b631eee4b2f7dcf07cd90 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Thu, 26 Mar 2026 12:48:08 -0700 Subject: [PATCH 1/2] [AIENG-404] The plumbing command enhancements for alias and component aggregation. --- smart_tests/__main__.py | 2 + smart_tests/args4p/typer/__init__.py | 6 ++- smart_tests/commands/record/build.py | 21 ++++++++ smart_tests/commands/update/__init__.py | 11 ++++ smart_tests/commands/update/alias.py | 40 ++++++++++++++ smart_tests/utils/commands.py | 1 + tests/commands/record/test_build.py | 71 +++++++++++++++++++++---- tests/commands/update/__init__.py | 0 tests/commands/update/test_alias.py | 69 ++++++++++++++++++++++++ 9 files changed, 210 insertions(+), 11 deletions(-) create mode 100644 smart_tests/commands/update/__init__.py create mode 100644 smart_tests/commands/update/alias.py create mode 100644 tests/commands/update/__init__.py create mode 100644 tests/commands/update/test_alias.py diff --git a/smart_tests/__main__.py b/smart_tests/__main__.py index 460325f7e..97120c3cf 100644 --- a/smart_tests/__main__.py +++ b/smart_tests/__main__.py @@ -13,10 +13,12 @@ from smart_tests.commands.record import record from smart_tests.commands.stats import stats from smart_tests.commands.subset import subset +from smart_tests.commands.update import update from smart_tests.commands.verify import verify cli = Group(name="cli", callback=Application) cli.add_command(record) +cli.add_command(update) cli.add_command(subset) # TODO: main.add_command(split_subset) cli.add_command(verify) diff --git a/smart_tests/args4p/typer/__init__.py b/smart_tests/args4p/typer/__init__.py index 4800aed21..0bbb09edc 100644 --- a/smart_tests/args4p/typer/__init__.py +++ b/smart_tests/args4p/typer/__init__.py @@ -33,9 +33,11 @@ def Argument( return _Argument(name=None, type=type, multiple=multiple, required=required, metavar=metavar, help=help, default=default) -class Exit(Exception): +class Exit(BaseException): ''' - Raise this exception to exit the CLI with the given exit code + Raise this exception to exit the CLI with the given exit code. + Extends BaseException (not Exception) so that broad `except Exception` handlers in user code + do not accidentally swallow it — analogous to Java's Error vs Exception distinction. ''' def __init__(self, code: int): diff --git a/smart_tests/commands/record/build.py b/smart_tests/commands/record/build.py index 2580bfd37..dafb8bb13 100644 --- a/smart_tests/commands/record/build.py +++ b/smart_tests/commands/record/build.py @@ -87,6 +87,13 @@ def build( help="Set external link of a title and url", type=parse_key_value, )] = [], + components: Annotated[List[KeyValue], typer.Option( + "--component", + multiple=True, + help="Include another build as a named component. Format: NAME=BUILD_OR_ALIAS", + metavar="NAME=BUILDNAME", + type=parse_key_value, + )] = [], ): # Parse key-value pairs for commits @@ -303,6 +310,19 @@ def send(ws: List[Workspace]) -> str | None: def compute_links(): return capture_links(link_options=links, env=os.environ) + 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] + try: lineage = branch or ws[0].branch if lineage is None: @@ -319,6 +339,7 @@ def compute_links(): } for w in ws], "links": compute_links(), "timestamp": parsed_timestamp.isoformat() if parsed_timestamp else None, + "components": compute_components(), } res = client.request("post", "builds", payload=payload) diff --git a/smart_tests/commands/update/__init__.py b/smart_tests/commands/update/__init__.py new file mode 100644 index 000000000..d0694dd44 --- /dev/null +++ b/smart_tests/commands/update/__init__.py @@ -0,0 +1,11 @@ +from ... import args4p +from ...app import Application +from .alias import alias + + +@args4p.group(help="Update Smart Tests resources") +def update(app: Application): + return app + + +update.add_command(alias) diff --git a/smart_tests/commands/update/alias.py b/smart_tests/commands/update/alias.py new file mode 100644 index 000000000..7512394aa --- /dev/null +++ b/smart_tests/commands/update/alias.py @@ -0,0 +1,40 @@ +from typing import Annotated + +import click +from requests import HTTPError + +import smart_tests.args4p.typer as typer +from smart_tests import args4p +from smart_tests.app import Application +from smart_tests.utils.commands import Command +from smart_tests.utils.fail_fast_mode import set_fail_fast_mode, warn_and_exit_if_fail_fast_mode +from smart_tests.utils.smart_tests_client import SmartTestsClient +from smart_tests.utils.tracking import TrackingClient + + +@args4p.command(help="Point an alias at a build") +def alias( + app: Application, + build_name: Annotated[str, typer.Option( + "--build", + help="Build name to point the alias at", + metavar="NAME", + required=True, + )], + alias_name: Annotated[str, typer.Option( + "--alias", + help="Alias name", + metavar="NAME", + required=True, + )], +): + tracking_client = TrackingClient(Command.UPDATE_ALIAS, app=app) + client = SmartTestsClient(app=app, tracking_client=tracking_client) + set_fail_fast_mode(client.is_fail_fast_mode()) + + try: + 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}'") + except Exception as e: + warn_and_exit_if_fail_fast_mode(f"Failed to update alias: {e}") diff --git a/smart_tests/utils/commands.py b/smart_tests/utils/commands.py index 7fda5bc1f..1a51097b6 100644 --- a/smart_tests/utils/commands.py +++ b/smart_tests/utils/commands.py @@ -10,6 +10,7 @@ class Command(Enum): COMMIT = 'COMMIT' DETECT_FLAKE = 'DETECT_FLAKE' GATE = 'GATE' + UPDATE_ALIAS = 'UPDATE_ALIAS' def display_name(self): return self.value.lower().replace('_', ' ') diff --git a/tests/commands/record/test_build.py b/tests/commands/record/test_build.py index ce4b47129..6276f32a3 100644 --- a/tests/commands/record/test_build.py +++ b/tests/commands/record/test_build.py @@ -71,7 +71,8 @@ def test_submodule(self, mock_check_output): }, ], "links": [], - "timestamp": None + "timestamp": None, + "components": [] }, payload) @responses.activate @@ -112,7 +113,8 @@ def test_no_submodule(self, mock_check_output): }, ], "links": [], - "timestamp": None + "timestamp": None, + "components": [] }, payload) @responses.activate @@ -151,7 +153,8 @@ def test_no_git_directory(self): }, ], "links": [], - "timestamp": None + "timestamp": None, + "components": [] }, payload) finally: @@ -191,7 +194,8 @@ def test_commit_option_and_build_option(self): }, ], "links": [], - 'timestamp': None + 'timestamp': None, + "components": [] }, payload) responses.calls.reset() @@ -223,7 +227,8 @@ def test_commit_option_and_build_option(self): }, ], "links": [], - "timestamp": None + "timestamp": None, + "components": [] }, payload) responses.calls.reset() @@ -255,7 +260,8 @@ def test_commit_option_and_build_option(self): }, ], "links": [], - "timestamp": None + "timestamp": None, + "components": [] }, payload) responses.calls.reset() self.assertIn("Invalid repository name B in a --branch option.", result.output) @@ -295,7 +301,8 @@ def test_commit_option_and_build_option(self): }, ], "links": [], - "timestamp": None + "timestamp": None, + "components": [] }, payload) responses.calls.reset() @@ -367,7 +374,8 @@ def test_with_timestamp(self, mock_check_output): }, ], "links": [], - "timestamp": "2025-01-23T12:34:56+00:00" + "timestamp": "2025-01-23T12:34:56+00:00", + "components": [] }, payload) @mock.patch.dict(os.environ, {"SMART_TESTS_TOKEN": CliTestCase.smart_tests_token}) @@ -412,7 +420,8 @@ def test_with_link(self): {"title": "url", "url": "https://smart-tests.test", "kind": "CUSTOM_LINK"}, {"title": "build", "url": "https://build.smart-tests.test", "kind": "CUSTOM_LINK"}, ], - "timestamp": None + "timestamp": None, + "components": [] }, payload) # with invalid kind @@ -505,3 +514,47 @@ def test_build_with_links(self): "--build", self.build_name) self.assert_success(result) + + @responses.activate + @mock.patch.dict(os.environ, {"SMART_TESTS_TOKEN": CliTestCase.smart_tests_token}) + @mock.patch.dict(os.environ, {"GITHUB_ACTIONS": ""}) + @mock.patch.dict(os.environ, {"GITHUB_PULL_REQUEST_URL": ""}) + def test_with_components(self): + result = self.cli( + "record", "build", + "--build", self.build_name, + "--branch", "main", + "--no-commit-collection", + "--commit", ".=abc123", + "--component", "payment=staging-payment-svc", + "--component", "auth=staging-auth-svc", + ) + self.assert_success(result) + + payload = json.loads(responses.calls[1].request.body.decode()) + self.assert_json_orderless_equal( + { + "buildNumber": "123", + "lineage": "main", + "commitHashes": [{"repositoryName": ".", "commitHash": "abc123", "branchName": ""}], + "links": [], + "timestamp": None, + "components": [ + {"name": "payment", "build": "staging-payment-svc"}, + {"name": "auth", "build": "staging-auth-svc"}, + ], + }, payload) + + @mock.patch.dict(os.environ, {"SMART_TESTS_TOKEN": CliTestCase.smart_tests_token}) + def test_duplicate_component_name(self): + result = self.cli( + "record", "build", + "--build", self.build_name, + "--branch", "main", + "--no-commit-collection", + "--commit", ".=abc123", + "--component", "payment=svc-a", + "--component", "payment=svc-b", + ) + self.assert_exit_code(result, 1) + self.assertIn("Duplicate component name: 'payment'", result.output) diff --git a/tests/commands/update/__init__.py b/tests/commands/update/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/commands/update/test_alias.py b/tests/commands/update/test_alias.py new file mode 100644 index 000000000..c33435c95 --- /dev/null +++ b/tests/commands/update/test_alias.py @@ -0,0 +1,69 @@ +import json +import os +from unittest import mock + +import responses # type: ignore + +from tests.cli_test_case import CliTestCase +from smart_tests.utils.http_client import get_base_url + + +class AliasTest(CliTestCase): + alias_name = "staging-payment-svc" + build_name_target = "jenkins-main-135" + error_body = "Build 'jenkins-main-135' not found" + + def alias_url(self): + return ( + f"{get_base_url()}/intake/organizations/{self.organization}" + f"/workspaces/{self.workspace}/builds/aliases/{self.alias_name}" + ) + + @responses.activate + @mock.patch.dict(os.environ, {"SMART_TESTS_TOKEN": CliTestCase.smart_tests_token}) + def test_update_alias(self): + responses.add(responses.PUT, self.alias_url(), json={}, status=200) + + result = self.cli( + "update", "alias", + "--build", self.build_name_target, + "--alias", self.alias_name, + ) + self.assert_success(result) + + put_call = next(c for c in responses.calls if c.request.method == "PUT") + self.assert_json_orderless_equal( + {"build": self.build_name_target}, + json.loads(put_call.request.body) + ) + self.assertIn(self.alias_name, result.output) + self.assertIn(self.build_name_target, result.output) + + @responses.activate + @mock.patch.dict(os.environ, {"SMART_TESTS_TOKEN": CliTestCase.smart_tests_token}) + def test_update_alias_build_not_found(self): + """Without fail-fast mode: prints warning in yellow, exits 0 (doesn't halt CI).""" + responses.add(responses.PUT, self.alias_url(), json={"reason": self.error_body}, status=404) + + result = self.cli( + "update", "alias", + "--build", self.build_name_target, + "--alias", self.alias_name, + ) + self.assert_success(result) + self.assertIn(self.error_body, result.output) + + @responses.activate + @mock.patch.dict(os.environ, {"SMART_TESTS_TOKEN": CliTestCase.smart_tests_token}) + @mock.patch("smart_tests.utils.fail_fast_mode.is_fail_fast_mode", return_value=True) + def test_update_alias_build_not_found_fail_fast(self, _mock_ffm): + """With fail-fast mode: prints error in red, exits 1.""" + responses.add(responses.PUT, self.alias_url(), json={"reason": self.error_body}, status=404) + + result = self.cli( + "update", "alias", + "--build", self.build_name_target, + "--alias", self.alias_name, + ) + self.assert_exit_code(result, 1) + self.assertIn(self.error_body, result.output) From e69105f33f7447f40f4deac9128069a101f84b14 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Mon, 30 Mar 2026 11:23:29 -0700 Subject: [PATCH 2/2] [AIENG-404] Addressing the code review comment from Copilot - Unused import statement - Reject path traversal attack --- smart_tests/commands/update/alias.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/smart_tests/commands/update/alias.py b/smart_tests/commands/update/alias.py index 7512394aa..1bc07a8e2 100644 --- a/smart_tests/commands/update/alias.py +++ b/smart_tests/commands/update/alias.py @@ -1,7 +1,6 @@ from typing import Annotated import click -from requests import HTTPError import smart_tests.args4p.typer as typer from smart_tests import args4p @@ -32,6 +31,15 @@ def alias( client = SmartTestsClient(app=app, tracking_client=tracking_client) set_fail_fast_mode(client.is_fail_fast_mode()) + # TODO: It's not entirely clear to me which layer is responsible for URL encoding + # this validation logic was copied from record/build.py + if "/" in alias_name or "%2f" in alias_name.lower(): + click.echo("--alias must not contain a slash and an encoded slash", err=True) + raise typer.Exit(1) + if "%25" in alias_name: + click.echo("--alias must not contain encoded % (%25)", err=True) + raise typer.Exit(1) + try: res = client.request("put", f"builds/aliases/{alias_name}", payload={"build": build_name}) res.raise_for_status()