diff --git a/cli/commands/configure.py b/cli/commands/configure.py index 7f4c5cb..1d0464d 100644 --- a/cli/commands/configure.py +++ b/cli/commands/configure.py @@ -20,12 +20,36 @@ PLUGINS = ["CKAN", "Socrata", "ArcGIS"] -# Bucket name must match the `backend "s3"` block in terraform/aws/main.tf. +# Default suggested bucket name shown in the wizard prompt. +# Users should change this to a unique name for their deployment. TERRAFORM_STATE_BUCKET = "opencontext-terraform-state" +def _check_state_bucket(bucket_name: str, region: str) -> str: + """Check whether the S3 bucket is available for use as Terraform state. + + Returns one of: + "ok" — bucket exists and belongs to this account + "missing" — bucket does not exist (will be created later) + "taken" — bucket exists but belongs to another account (403) + + Any other ClientError is re-raised unchanged. + """ + s3 = boto3.client("s3", region_name=region) + try: + s3.head_bucket(Bucket=bucket_name) + return "ok" + except botocore.exceptions.ClientError as e: + error_code = e.response["Error"]["Code"] + if error_code in ("403", "AccessDenied"): + return "taken" + if error_code in ("404", "NoSuchBucket"): + return "missing" + raise + + def _ensure_state_bucket(bucket_name: str, region: str) -> None: - """Check that the Terraform S3 state bucket exists; create it if not. + """Create the Terraform S3 state bucket if it does not already exist. Versioning and server-side encryption are enabled on newly created buckets. No DynamoDB table is created — the Terraform backend does not use state @@ -273,6 +297,27 @@ def configure( if region is None: raise typer.Exit(0) + # Prompt for state bucket name and validate immediately. + # Re-prompt if the bucket is owned by another AWS account (403). + # The --state-bucket CLI flag skips this prompt when a non-default value + # is passed, allowing automation and re-runs against existing state. + if state_bucket == TERRAFORM_STATE_BUCKET: + while True: + state_bucket = questionary.text( + "Terraform state bucket name:", + default=TERRAFORM_STATE_BUCKET, + ).ask() + if state_bucket is None: + raise typer.Exit(0) + status = _check_state_bucket(state_bucket, region) + if status == "taken": + console.print( + f"[red]Bucket [bold]{state_bucket}[/bold] is owned by another " + f"AWS account. Please choose a different name.[/red]" + ) + continue + break + city_slug = city_name.lower().replace(" ", "-") suggested_lambda = f"{city_slug}-opencontext-mcp-{env}" @@ -350,20 +395,20 @@ def configure( _ensure_state_bucket(state_bucket, region) - # Override the backend config at init time so Terraform uses the correct - # bucket and region instead of the defaults hard-coded in main.tf. - if not (terraform_dir / ".terraform").exists(): - init_cmd = [ - "terraform", - "init", - f"-backend-config=bucket={state_bucket}", - f"-backend-config=region={region}", - ] - run_cmd( - init_cmd, - cwd=terraform_dir, - spinner_msg="Initializing Terraform", - ) + # Always reinitialize with -reconfigure to ensure the backend config is + # up to date, even if .terraform already exists from a previous run. + init_cmd = [ + "terraform", + "init", + "-reconfigure", + f"-backend-config=bucket={state_bucket}", + f"-backend-config=region={region}", + ] + run_cmd( + init_cmd, + cwd=terraform_dir, + spinner_msg="Initializing Terraform", + ) result = subprocess.run( ["terraform", "workspace", "list"], diff --git a/tests/test_cli_configure.py b/tests/test_cli_configure.py index bbba90d..05c3df3 100644 --- a/tests/test_cli_configure.py +++ b/tests/test_cli_configure.py @@ -525,6 +525,7 @@ def _side(*a, **kw): "City", "120", "us-east-1", + "opencontext-terraform-state", # state bucket name prompt "city-mcp-staging", "512", "120", @@ -602,6 +603,10 @@ def _run_configure_wizard(tmp_path, extra_kwargs: dict | None = None): mock_s3 = MagicMock() mock_s3.head_bucket.return_value = {} + # Only include bucket name response if not passing --state-bucket explicitly + skip_bucket_prompt = extra_kwargs and "state_bucket" in extra_kwargs + bucket_responses = [] if skip_bucket_prompt else ["opencontext-terraform-state"] + with ( patch("cli.commands.configure.get_project_root", return_value=tmp_path), patch("cli.commands.configure.get_terraform_dir", return_value=tf_dir), @@ -622,6 +627,7 @@ def _run_configure_wizard(tmp_path, extra_kwargs: dict | None = None): "City", "120", "us-east-1", + *bucket_responses, "city-mcp-staging", "512", "120", @@ -636,7 +642,7 @@ def _run_configure_wizard(tmp_path, extra_kwargs: dict | None = None): configure(**(extra_kwargs or {})) return mock_run_cmd.call_args_list, mock_s3 - + class TestStateBucketFlag: """Tests for the --state-bucket CLI option on configure().""" @@ -647,12 +653,14 @@ def test_default_bucket_passed_to_ensure_state_bucket(self, tmp_path): _, mock_s3 = _run_configure_wizard(tmp_path) - # boto3.client is called inside _ensure_state_bucket; head_bucket receives - # the bucket name via the Bucket kwarg. - mock_s3.head_bucket.assert_called_once_with(Bucket=TERRAFORM_STATE_BUCKET) + # head_bucket is called twice: once in _check_state_bucket (validation prompt) + # and once in _ensure_state_bucket (creation check). Verify at least one call + # used the default bucket name. + mock_s3.head_bucket.assert_any_call(Bucket=TERRAFORM_STATE_BUCKET) def test_custom_bucket_passed_to_ensure_state_bucket(self, tmp_path): - """When --state-bucket is provided, _ensure_state_bucket receives the custom name.""" + """When --state-bucket is provided, the prompt is skipped and _ensure_state_bucket + receives the custom name directly.""" custom = "my-custom-tf-state" _, mock_s3 = _run_configure_wizard(tmp_path, {"state_bucket": custom}) diff --git a/tests/test_cli_configure_extended.py b/tests/test_cli_configure_extended.py index 465cd39..227d579 100644 --- a/tests/test_cli_configure_extended.py +++ b/tests/test_cli_configure_extended.py @@ -35,10 +35,12 @@ class TestConfigureWizardCKAN: @patch("cli.commands.configure.run_cmd") @patch("cli.commands.configure.get_project_root") @patch("cli.commands.configure.get_terraform_dir") + @patch("cli.commands.configure.boto3.client") @patch("cli.commands.configure.questionary") def test_ckan_wizard_writes_config_and_tfvars( self, mock_q, + mock_boto3, mock_tf_dir, mock_root, mock_run_cmd, @@ -52,6 +54,10 @@ def test_ckan_wizard_writes_config_and_tfvars( mock_root.return_value = tmp_path mock_tf_dir.return_value = tf_dir + mock_s3 = MagicMock() + mock_s3.head_bucket.return_value = {} + mock_boto3.return_value = mock_s3 + responses = [ "Use example config as template", # starting_point "City of Boston", # org_name @@ -63,6 +69,7 @@ def test_ckan_wizard_writes_config_and_tfvars( "Boston", # city_name (plugin) "120", # timeout "us-east-1", # region + "opencontext-terraform-state", # state bucket name prompt "boston-opencontext-mcp-staging", # lambda_name "512", # lambda_memory "120", # lambda_timeout @@ -82,9 +89,10 @@ def test_ckan_wizard_writes_config_and_tfvars( responses[10], responses[11], responses[12], + responses[13], ] ) - mock_q.confirm.side_effect = _mock_q([responses[13]]) + mock_q.confirm.side_effect = _mock_q([responses[14]]) # terraform workspace list returns empty mock_subproc.return_value = subprocess.CompletedProcess( @@ -105,10 +113,12 @@ def test_ckan_wizard_writes_config_and_tfvars( @patch("cli.commands.configure.run_cmd") @patch("cli.commands.configure.get_project_root") @patch("cli.commands.configure.get_terraform_dir") + @patch("cli.commands.configure.boto3.client") @patch("cli.commands.configure.questionary") def test_socrata_wizard_without_app_token( self, mock_q, + mock_boto3, mock_tf_dir, mock_root, mock_run_cmd, @@ -122,6 +132,10 @@ def test_socrata_wizard_without_app_token( mock_root.return_value = tmp_path mock_tf_dir.return_value = tf_dir + mock_s3 = MagicMock() + mock_s3.head_bucket.return_value = {} + mock_boto3.return_value = mock_s3 + mock_q.select.side_effect = _mock_q( ["Start from scratch", "staging", "Socrata"] ) @@ -133,6 +147,7 @@ def test_socrata_wizard_without_app_token( "", # app_token (empty → omitted) "120", # timeout "us-east-1", # region + "opencontext-terraform-state", # state bucket name prompt "chicago-mcp-staging", # lambda_name "512", # lambda_memory "120", # lambda_timeout @@ -155,10 +170,12 @@ def test_socrata_wizard_without_app_token( @patch("cli.commands.configure.run_cmd") @patch("cli.commands.configure.get_project_root") @patch("cli.commands.configure.get_terraform_dir") + @patch("cli.commands.configure.boto3.client") @patch("cli.commands.configure.questionary") def test_arcgis_wizard_with_custom_domain( self, mock_q, + mock_boto3, mock_tf_dir, mock_root, mock_run_cmd, @@ -172,6 +189,10 @@ def test_arcgis_wizard_with_custom_domain( mock_root.return_value = tmp_path mock_tf_dir.return_value = tf_dir + mock_s3 = MagicMock() + mock_s3.head_bucket.return_value = {} + mock_boto3.return_value = mock_s3 + mock_q.select.side_effect = _mock_q(["Start from scratch", "prod", "ArcGIS"]) mock_q.text.side_effect = _mock_q( [ @@ -181,6 +202,7 @@ def test_arcgis_wizard_with_custom_domain( "Seattle", # city_name (plugin) "120", # timeout "us-west-2", # region + "opencontext-terraform-state", # state bucket name prompt "seattle-mcp-prod", # lambda_name "512", # lambda_memory "120", # lambda_timeout @@ -209,10 +231,12 @@ def test_arcgis_wizard_with_custom_domain( @patch("cli.commands.configure.run_cmd") @patch("cli.commands.configure.get_project_root") @patch("cli.commands.configure.get_terraform_dir") + @patch("cli.commands.configure.boto3.client") @patch("cli.commands.configure.questionary") def test_wizard_selects_existing_workspace( self, mock_q, + mock_boto3, mock_tf_dir, mock_root, mock_run_cmd, @@ -227,6 +251,10 @@ def test_wizard_selects_existing_workspace( mock_root.return_value = tmp_path mock_tf_dir.return_value = tf_dir + mock_s3 = MagicMock() + mock_s3.head_bucket.return_value = {} + mock_boto3.return_value = mock_s3 + mock_q.select.side_effect = _mock_q(["Start from scratch", "staging", "CKAN"]) mock_q.text.side_effect = _mock_q( [ @@ -237,6 +265,7 @@ def test_wizard_selects_existing_workspace( "Boston", "120", "us-east-1", + "opencontext-terraform-state", # state bucket name prompt "boston-mcp-staging", "512", "120", @@ -260,10 +289,12 @@ def test_wizard_selects_existing_workspace( @patch("cli.commands.configure.run_cmd") @patch("cli.commands.configure.get_project_root") @patch("cli.commands.configure.get_terraform_dir") + @patch("cli.commands.configure.boto3.client") @patch("cli.commands.configure.questionary") def test_wizard_runs_terraform_init_if_needed( self, mock_q, + mock_boto3, mock_tf_dir, mock_root, mock_run_cmd, @@ -279,6 +310,10 @@ def test_wizard_runs_terraform_init_if_needed( mock_root.return_value = tmp_path mock_tf_dir.return_value = tf_dir + mock_s3 = MagicMock() + mock_s3.head_bucket.return_value = {} + mock_boto3.return_value = mock_s3 + mock_q.select.side_effect = _mock_q(["Start from scratch", "staging", "CKAN"]) mock_q.text.side_effect = _mock_q( [ @@ -289,6 +324,7 @@ def test_wizard_runs_terraform_init_if_needed( "City", "120", "us-east-1", + "opencontext-terraform-state", # state bucket name prompt "city-mcp-staging", "512", "120",