Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion packages/reflex-hosting-cli/src/reflex_cli/v2/gcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
tree is never modified. The script reads its parameters from environment
variables (GCP_PROJECT, GCP_REGION, SERVICE_NAME, AR_REPO, VERSION,
CLOUD_RUN_CPU, CLOUD_RUN_MEMORY, CLOUD_RUN_MIN_INSTANCES,
REFLEX_CLOUDBUILD_YAML).
CLOUD_RUN_SERVICE_ACCOUNT, REFLEX_CLOUDBUILD_YAML).
"""

from __future__ import annotations
Expand Down Expand Up @@ -41,6 +41,7 @@
ENV_CPU = "CLOUD_RUN_CPU"
ENV_MEMORY = "CLOUD_RUN_MEMORY"
ENV_MIN_INSTANCES = "CLOUD_RUN_MIN_INSTANCES"
ENV_SERVICE_ACCOUNT = "CLOUD_RUN_SERVICE_ACCOUNT"
# Path to the Cloud Build config file written by the CLI. The rewritten
# deploy script references it as ``--config="${REFLEX_CLOUDBUILD_YAML}"``.
ENV_REFLEX_CLOUDBUILD_YAML = "REFLEX_CLOUDBUILD_YAML"
Expand Down Expand Up @@ -162,6 +163,12 @@
type=click.IntRange(min=0),
help="Minimum number of Cloud Run instances to keep warm (sets CLOUD_RUN_MIN_INSTANCES). Set to 0 to scale to zero.",
)
@click.option(
"--service-account",
"service_account",
default=None,
help="IAM service account email the Cloud Run service runs as (sets CLOUD_RUN_SERVICE_ACCOUNT). If omitted, Cloud Run uses the project's default compute SA. The deploying principal needs roles/iam.serviceAccountUser on the target SA.",
)
@click.option(
"--source",
"source_dir",
Expand Down Expand Up @@ -199,6 +206,7 @@ def deploy_command(
cpu: str,
memory: str,
min_instances: int,
service_account: str | None,
source_dir: str,
token: str | None,
interactive: bool,
Expand Down Expand Up @@ -285,6 +293,8 @@ def deploy_command(
ENV_MEMORY: memory,
ENV_MIN_INSTANCES: str(min_instances),
}
if service_account:
deploy_env[ENV_SERVICE_ACCOUNT] = service_account
Comment on lines +296 to +297
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 If a user passes --service-account "" (an explicit empty string), the falsy check silently skips the injection and falls back to the default compute SA with no warning. The user gets no indication their flag was ignored. Using is not None is more precise here — it fires only for the truly-absent case and preserves the semantics of an explicit (even if invalid) user input triggering either a forward or an early error.

Suggested change
if service_account:
deploy_env[ENV_SERVICE_ACCOUNT] = service_account
if service_account is not None:
if not service_account:
console.error("--service-account cannot be an empty string.")
raise click.exceptions.Exit(2)
deploy_env[ENV_SERVICE_ACCOUNT] = service_account

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this one, i agree. a common case would be passing --service-account "$MY_SERVICE_ACCOUNT" in some CI job, and not realizing that the CI environment isn't defining MY_SERVICE_ACCOUNT`, so it's getting passed as empty string and resolving to the original behavior, despite user intent.


console.info("Received deploy manifest from Reflex.")
console.print("")
Expand Down
50 changes: 50 additions & 0 deletions tests/units/reflex_cli/v2/test_gcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,56 @@ def test_gcp_deploy_resource_flags_have_defaults(mocker: MockFixture, tmp_path:
assert env_overrides["CLOUD_RUN_MIN_INSTANCES"] == "1"


def test_gcp_deploy_forwards_service_account(mocker: MockFixture, tmp_path: Path):
"""--service-account threads through to CLOUD_RUN_SERVICE_ACCOUNT."""
run_mock = _patch_environment(mocker)
_mock_manifest_response(mocker)

result = runner.invoke(
hosting_cli,
[
"deploy",
"--gcp",
"--gcp-project",
"p",
"--source",
str(tmp_path),
"--service-account",
"my-sa@p.iam.gserviceaccount.com",
],
input="y\n",
)

assert result.exit_code == 0, result.output
env_overrides = run_mock.call_args.kwargs["env_overrides"]
assert (
env_overrides["CLOUD_RUN_SERVICE_ACCOUNT"] == "my-sa@p.iam.gserviceaccount.com"
)


def test_gcp_deploy_omits_service_account_when_unset(
mocker: MockFixture, tmp_path: Path
):
"""Without --service-account, CLOUD_RUN_SERVICE_ACCOUNT is not in env_overrides.

The deploy script defaults it to empty, so Cloud Run falls back to the
project's default compute SA. Leaving the key out of env_overrides (rather
than sending an empty string) keeps the dry-run output tidy.
"""
run_mock = _patch_environment(mocker)
_mock_manifest_response(mocker)

result = runner.invoke(
hosting_cli,
["deploy", "--gcp", "--gcp-project", "p", "--source", str(tmp_path)],
input="y\n",
)

assert result.exit_code == 0, result.output
env_overrides = run_mock.call_args.kwargs["env_overrides"]
assert "CLOUD_RUN_SERVICE_ACCOUNT" not in env_overrides


def test_gcp_deploy_rejects_negative_min_instances(mocker: MockFixture, tmp_path: Path):
"""--min-instances is IntRange(min=0); negative values fail at the CLI layer."""
run_mock = _patch_environment(mocker)
Expand Down
Loading