diff --git a/src/google/adk/cli/cli_deploy.py b/src/google/adk/cli/cli_deploy.py index 2632af9b..e48b9ecf 100644 --- a/src/google/adk/cli/cli_deploy.py +++ b/src/google/adk/cli/cli_deploy.py @@ -95,6 +95,52 @@ def _resolve_project(project_in_option: Optional[str]) -> str: return project +def _validate_gcloud_extra_args( + extra_gcloud_args: Optional[tuple[str, ...]], adk_managed_args: set[str] +) -> None: + """Validates that extra gcloud args don't conflict with ADK-managed args. + + This function dynamically checks for conflicts based on the actual args + that ADK will set, rather than using a hardcoded list. + + Args: + extra_gcloud_args: User-provided extra arguments for gcloud. + adk_managed_args: Set of argument names that ADK will set automatically. + Should include '--' prefix (e.g., '--project'). + + Raises: + click.ClickException: If any conflicts are found. + """ + if not extra_gcloud_args: + return + + # Parse user arguments into a set of argument names for faster lookup + user_arg_names = set() + for arg in extra_gcloud_args: + if arg.startswith('--'): + # Handle both '--arg=value' and '--arg value' formats + arg_name = arg.split('=')[0] + user_arg_names.add(arg_name) + + # Check for conflicts with ADK-managed args + conflicts = user_arg_names.intersection(adk_managed_args) + + if conflicts: + conflict_list = ', '.join(f"'{arg}'" for arg in sorted(conflicts)) + if len(conflicts) == 1: + raise click.ClickException( + f"The argument {conflict_list} conflicts with ADK's automatic" + ' configuration. ADK will set this argument automatically, so please' + ' remove it from your command.' + ) + else: + raise click.ClickException( + f"The arguments {conflict_list} conflict with ADK's automatic" + ' configuration. ADK will set these arguments automatically, so' + ' please remove them from your command.' + ) + + def _get_service_option_by_adk_version( adk_version: str, session_uri: Optional[str], @@ -141,6 +187,7 @@ def to_cloud_run( artifact_service_uri: Optional[str] = None, memory_service_uri: Optional[str] = None, a2a: bool = False, + extra_gcloud_args: Optional[tuple[str, ...]] = None, ): """Deploys an agent to Google Cloud Run. @@ -234,26 +281,56 @@ def to_cloud_run( click.echo('Deploying to Cloud Run...') region_options = ['--region', region] if region else [] project = _resolve_project(project) - subprocess.run( - [ - 'gcloud', - 'run', - 'deploy', - service_name, - '--source', - temp_folder, - '--project', - project, - *region_options, - '--port', - str(port), - '--verbosity', - log_level.lower() if log_level else verbosity, - '--labels', - 'created-by=adk', - ], - check=True, - ) + + # Build the set of args that ADK will manage + adk_managed_args = {'--source', '--project', '--port', '--verbosity'} + if region: + adk_managed_args.add('--region') + + # Validate that extra gcloud args don't conflict with ADK-managed args + _validate_gcloud_extra_args(extra_gcloud_args, adk_managed_args) + + # Build the command with extra gcloud args + gcloud_cmd = [ + 'gcloud', + 'run', + 'deploy', + service_name, + '--source', + temp_folder, + '--project', + project, + *region_options, + '--port', + str(port), + '--verbosity', + log_level.lower() if log_level else verbosity, + ] + + # Handle labels specially - merge user labels with ADK label + user_labels = [] + extra_args_without_labels = [] + + if extra_gcloud_args: + for arg in extra_gcloud_args: + if arg.startswith('--labels='): + # Extract user-provided labels + user_labels_value = arg[9:] # Remove '--labels=' prefix + user_labels.append(user_labels_value) + else: + extra_args_without_labels.append(arg) + + # Combine ADK label with user labels + all_labels = ['created-by=adk'] + all_labels.extend(user_labels) + labels_arg = ','.join(all_labels) + + gcloud_cmd.extend(['--labels', labels_arg]) + + # Add any remaining extra passthrough args + gcloud_cmd.extend(extra_args_without_labels) + + subprocess.run(gcloud_cmd, check=True) finally: click.echo(f'Cleaning up the temp folder: {temp_folder}') shutil.rmtree(temp_folder) diff --git a/src/google/adk/cli/cli_tools_click.py b/src/google/adk/cli/cli_tools_click.py index 3000edf7..9146de5d 100644 --- a/src/google/adk/cli/cli_tools_click.py +++ b/src/google/adk/cli/cli_tools_click.py @@ -858,7 +858,13 @@ def cli_api_server( server.run() -@deploy.command("cloud_run") +@deploy.command( + "cloud_run", + context_settings={ + "allow_extra_args": True, + "allow_interspersed_args": False, + }, +) @click.option( "--project", type=str, @@ -971,7 +977,9 @@ def cli_api_server( # TODO: Add eval_storage_uri option back when evals are supported in Cloud Run. @adk_services_options() @deprecated_adk_services_options() +@click.pass_context def cli_deploy_cloud_run( + ctx, agent: str, project: Optional[str], region: Optional[str], @@ -996,9 +1004,13 @@ def cli_deploy_cloud_run( AGENT: The path to the agent source code folder. - Example: + Use '--' to separate gcloud arguments from adk arguments. + + Examples: adk deploy cloud_run --project=[project] --region=[region] path/to/my_agent + + adk deploy cloud_run --project=[project] --region=[region] path/to/my_agent -- --no-allow-unauthenticated --min-instances=2 """ if verbosity: click.secho( @@ -1010,6 +1022,36 @@ def cli_deploy_cloud_run( session_service_uri = session_service_uri or session_db_url artifact_service_uri = artifact_service_uri or artifact_storage_uri + + # Parse arguments to separate gcloud args (after --) from regular args + gcloud_args = [] + if "--" in ctx.args: + separator_index = ctx.args.index("--") + gcloud_args = ctx.args[separator_index + 1 :] + regular_args = ctx.args[:separator_index] + + # If there are regular args before --, that's an error + if regular_args: + click.secho( + "Error: Unexpected arguments after agent path and before '--':" + f" {' '.join(regular_args)}. \nOnly arguments after '--' are passed" + " to gcloud.", + fg="red", + err=True, + ) + ctx.exit(2) + else: + # No -- separator, treat all args as an error to enforce the new behavior + if ctx.args: + click.secho( + f"Error: Unexpected arguments: {' '.join(ctx.args)}. \nUse '--' to" + " separate gcloud arguments, e.g.: adk deploy cloud_run [options]" + " agent_path -- --min-instances=2", + fg="red", + err=True, + ) + ctx.exit(2) + try: cli_deploy.to_cloud_run( agent_folder=agent, @@ -1029,6 +1071,7 @@ def cli_deploy_cloud_run( artifact_service_uri=artifact_service_uri, memory_service_uri=memory_service_uri, a2a=a2a, + extra_gcloud_args=tuple(gcloud_args), ) except Exception as e: click.secho(f"Deploy failed: {e}", fg="red", err=True) diff --git a/tests/unittests/cli/test_cli_tools_click_option_mismatch.py b/tests/unittests/cli/test_cli_tools_click_option_mismatch.py index 3a01c469..346fd421 100644 --- a/tests/unittests/cli/test_cli_tools_click_option_mismatch.py +++ b/tests/unittests/cli/test_cli_tools_click_option_mismatch.py @@ -137,7 +137,7 @@ def test_adk_deploy_cloud_run(): cloud_run_command, cli_deploy_cloud_run.callback, "deploy cloud_run", - ignore_params={"verbose"}, + ignore_params={"verbose", "ctx"}, ) diff --git a/tests/unittests/cli/utils/test_cli_deploy.py b/tests/unittests/cli/utils/test_cli_deploy.py index dfcbf076..9b13c87f 100644 --- a/tests/unittests/cli/utils/test_cli_deploy.py +++ b/tests/unittests/cli/utils/test_cli_deploy.py @@ -636,3 +636,76 @@ def test_to_gke_happy_path( # 4. Verify cleanup assert str(rmtree_recorder.get_last_call_args()[0]) == str(tmp_path) + + +# Label merging tests +@pytest.mark.parametrize( + "extra_gcloud_args, expected_labels", + [ + # No user labels - should only have default ADK label + (None, "created-by=adk"), + ([], "created-by=adk"), + # Single user label + (["--labels=env=test"], "created-by=adk,env=test"), + # Multiple user labels in same argument + ( + ["--labels=env=test,team=myteam"], + "created-by=adk,env=test,team=myteam", + ), + # User labels mixed with other args + ( + ["--memory=1Gi", "--labels=env=test", "--cpu=1"], + "created-by=adk,env=test", + ), + # Multiple --labels arguments + ( + ["--labels=env=test", "--labels=team=myteam"], + "created-by=adk,env=test,team=myteam", + ), + # Labels with other passthrough args + ( + ["--timeout=300", "--labels=env=prod", "--max-instances=10"], + "created-by=adk,env=prod", + ), + ], +) +def test_cloud_run_label_merging( + monkeypatch: pytest.MonkeyPatch, + agent_dir: Callable[[bool, bool], Path], + tmp_path: Path, + extra_gcloud_args: list[str] | None, + expected_labels: str, +) -> None: + """Test that user labels are properly merged with the default ADK label.""" + src_dir = agent_dir(False, False) + run_recorder = _Recorder() + + monkeypatch.setattr(subprocess, "run", run_recorder) + monkeypatch.setattr(shutil, "rmtree", lambda x: None) + + # Execute the function under test + cli_deploy.to_cloud_run( + agent_folder=str(src_dir), + project="test-project", + region="us-central1", + service_name="test-service", + app_name="test-app", + temp_folder=str(tmp_path), + port=8080, + trace_to_cloud=False, + with_ui=False, + log_level="info", + verbosity="info", + adk_version="1.0.0", + extra_gcloud_args=tuple(extra_gcloud_args) if extra_gcloud_args else None, + ) + + # Verify that the gcloud command was called + assert len(run_recorder.calls) == 1 + gcloud_args = run_recorder.get_last_call_args()[0] + + # Find the labels argument + labels_idx = gcloud_args.index("--labels") + actual_labels = gcloud_args[labels_idx + 1] + + assert actual_labels == expected_labels diff --git a/tests/unittests/cli/utils/test_cli_tools_click.py b/tests/unittests/cli/utils/test_cli_tools_click.py index b57097ab..138289ed 100644 --- a/tests/unittests/cli/utils/test_cli_tools_click.py +++ b/tests/unittests/cli/utils/test_cli_tools_click.py @@ -196,6 +196,148 @@ def test_cli_deploy_cloud_run_failure( assert "Deploy failed: boom" in result.output +def test_cli_deploy_cloud_run_passthrough_args( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Extra args after '--' should be passed through to the gcloud command.""" + rec = _Recorder() + monkeypatch.setattr(cli_tools_click.cli_deploy, "to_cloud_run", rec) + + agent_dir = tmp_path / "agent_passthrough" + agent_dir.mkdir() + runner = CliRunner() + result = runner.invoke( + cli_tools_click.main, + [ + "deploy", + "cloud_run", + "--project", + "test-project", + "--region", + "us-central1", + str(agent_dir), + "--", + "--labels=test-label=test", + "--memory=1Gi", + "--cpu=1", + ], + ) + # Print debug information if the test fails + if result.exit_code != 0: + print(f"Exit code: {result.exit_code}") + print(f"Output: {result.output}") + print(f"Exception: {result.exception}") + + assert result.exit_code == 0 + assert rec.calls, "cli_deploy.to_cloud_run must be invoked" + + # Check that extra_gcloud_args were passed correctly + called_kwargs = rec.calls[0][1] + extra_args = called_kwargs.get("extra_gcloud_args") + assert extra_args is not None + assert "--labels=test-label=test" in extra_args + assert "--memory=1Gi" in extra_args + assert "--cpu=1" in extra_args + + +def test_cli_deploy_cloud_run_rejects_args_without_separator( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Args without '--' separator should be rejected with helpful error message.""" + rec = _Recorder() + monkeypatch.setattr(cli_tools_click.cli_deploy, "to_cloud_run", rec) + + agent_dir = tmp_path / "agent_no_sep" + agent_dir.mkdir() + runner = CliRunner() + result = runner.invoke( + cli_tools_click.main, + [ + "deploy", + "cloud_run", + "--project", + "test-project", + "--region", + "us-central1", + str(agent_dir), + "--labels=test-label=test", # This should be rejected + ], + ) + + assert result.exit_code == 2 + assert "Unexpected arguments:" in result.output + assert "Use '--' to separate gcloud arguments" in result.output + assert not rec.calls, "cli_deploy.to_cloud_run should not be called" + + +def test_cli_deploy_cloud_run_rejects_args_before_separator( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Args before '--' separator should be rejected.""" + rec = _Recorder() + monkeypatch.setattr(cli_tools_click.cli_deploy, "to_cloud_run", rec) + + agent_dir = tmp_path / "agent_before_sep" + agent_dir.mkdir() + runner = CliRunner() + result = runner.invoke( + cli_tools_click.main, + [ + "deploy", + "cloud_run", + "--project", + "test-project", + "--region", + "us-central1", + str(agent_dir), + "unexpected_arg", # This should be rejected + "--", + "--labels=test-label=test", + ], + ) + + assert result.exit_code == 2 + assert ( + "Unexpected arguments after agent path and before '--':" in result.output + ) + assert "unexpected_arg" in result.output + assert not rec.calls, "cli_deploy.to_cloud_run should not be called" + + +def test_cli_deploy_cloud_run_allows_empty_gcloud_args( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """No gcloud args after '--' should be allowed.""" + rec = _Recorder() + monkeypatch.setattr(cli_tools_click.cli_deploy, "to_cloud_run", rec) + + agent_dir = tmp_path / "agent_empty_gcloud" + agent_dir.mkdir() + runner = CliRunner() + result = runner.invoke( + cli_tools_click.main, + [ + "deploy", + "cloud_run", + "--project", + "test-project", + "--region", + "us-central1", + str(agent_dir), + "--", + # No gcloud args after -- + ], + ) + + assert result.exit_code == 0 + assert rec.calls, "cli_deploy.to_cloud_run must be invoked" + + # Check that extra_gcloud_args is empty + called_kwargs = rec.calls[0][1] + extra_args = called_kwargs.get("extra_gcloud_args") + assert extra_args == () + + # cli deploy agent_engine def test_cli_deploy_agent_engine_success( tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -476,3 +618,96 @@ def test_cli_eval_with_eval_set_id( app_name=app_name ) assert len(eval_set_results) == 2 + + +def test_cli_deploy_cloud_run_gcloud_arg_conflict( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Extra gcloud args that conflict with ADK deploy args should raise ClickException.""" + + def _mock_to_cloud_run(*_a, **kwargs): + # Import and call the validation function + from google.adk.cli.cli_deploy import _validate_gcloud_extra_args + + # Build the same set of managed args as the real function would + adk_managed_args = {"--source", "--project", "--port", "--verbosity"} + if kwargs.get("region"): + adk_managed_args.add("--region") + _validate_gcloud_extra_args( + kwargs.get("extra_gcloud_args"), adk_managed_args + ) + + monkeypatch.setattr( + cli_tools_click.cli_deploy, "to_cloud_run", _mock_to_cloud_run + ) + + agent_dir = tmp_path / "agent_conflict" + agent_dir.mkdir() + runner = CliRunner() + + # Test with conflicting --project arg + result = runner.invoke( + cli_tools_click.main, + [ + "deploy", + "cloud_run", + "--project", + "test-project", + "--region", + "us-central1", + str(agent_dir), + "--", + "--project=conflict-project", # This should conflict + ], + ) + + expected_msg = ( + "The argument '--project' conflicts with ADK's automatic configuration." + " ADK will set this argument automatically, so please remove it from your" + " command." + ) + assert expected_msg in result.output + + # Test with conflicting --port arg + result = runner.invoke( + cli_tools_click.main, + [ + "deploy", + "cloud_run", + "--project", + "test-project", + str(agent_dir), + "--", + "--port=9000", # This should conflict + ], + ) + + expected_msg = ( + "The argument '--port' conflicts with ADK's automatic configuration. ADK" + " will set this argument automatically, so please remove it from your" + " command." + ) + assert expected_msg in result.output + + # Test with conflicting --region arg + result = runner.invoke( + cli_tools_click.main, + [ + "deploy", + "cloud_run", + "--project", + "test-project", + "--region", + "us-central1", + str(agent_dir), + "--", + "--region=us-west1", # This should conflict + ], + ) + + expected_msg = ( + "The argument '--region' conflicts with ADK's automatic configuration." + " ADK will set this argument automatically, so please remove it from your" + " command." + ) + assert expected_msg in result.output