feat(cli): support remote GCS state fetching - #174
Conversation
There was a problem hiding this comment.
Code Review
This pull request transitions the configuration from using 'namespace' to 'instance_name' across Terraform files and the admin CLI, while maintaining backward compatibility. It also adds support for fetching Terraform outputs directly from GCS remote state files. The review feedback highlights several opportunities to make the remote state retrieval more robust, such as properly handling whitespace-only state locations, ensuring the parsed JSON state is a dictionary, and passing the project ID to the GCS client, along with adding corresponding assertions in the unit tests.
| if tf_state_location: | ||
| gcs_uri = tf_state_location.strip() | ||
| if not gcs_uri.endswith(".tfstate"): | ||
| gcs_uri = gcs_uri.rstrip("/") + "/default.tfstate" |
There was a problem hiding this comment.
If tf_state_location is a whitespace-only string (e.g., " "), bool(tf_state_location) evaluates to True, which bypasses the elif project_id and instance_name fallback. It then strips to an empty string and appends /default.tfstate, resulting in an invalid GCS URI error. We should strip the string first and check if it is non-empty to allow falling back to the project_id and instance_name logic if they are provided.
| if tf_state_location: | |
| gcs_uri = tf_state_location.strip() | |
| if not gcs_uri.endswith(".tfstate"): | |
| gcs_uri = gcs_uri.rstrip("/") + "/default.tfstate" | |
| tf_state_location_stripped = tf_state_location.strip() if tf_state_location else "" | |
| if tf_state_location_stripped: | |
| gcs_uri = tf_state_location_stripped | |
| if not gcs_uri.endswith(".tfstate"): | |
| gcs_uri = gcs_uri.rstrip("/") + "/default.tfstate" |
| try: | ||
| client = storage.Client() | ||
| except Exception as e: |
There was a problem hiding this comment.
When initializing the storage.Client, it is highly recommended to pass the project_id if it is available. This ensures that the client is explicitly bound to the target GCP project for quota and billing purposes, rather than relying on the ambient default project of the active gcloud credentials.
| try: | |
| client = storage.Client() | |
| except Exception as e: | |
| try: | |
| client = storage.Client(project=project_id) if project_id else storage.Client() | |
| except Exception as e: |
| try: | ||
| state_data = json.loads(content) | ||
| except json.JSONDecodeError: | ||
| raise click.ClickException(f"Failed to parse Terraform state at '{gcs_uri}' as valid JSON.") | ||
|
|
||
| outputs = state_data.get("outputs", {}) |
There was a problem hiding this comment.
If the downloaded GCS file is valid JSON but not a dictionary (e.g., a JSON list or primitive), calling state_data.get("outputs", {}) will raise an AttributeError. We should defensively verify that state_data is a dictionary.
try:
state_data = json.loads(content)
except json.JSONDecodeError:
raise click.ClickException(f"Failed to parse Terraform state at '{gcs_uri}' as valid JSON.")
if not isinstance(state_data, dict):
raise click.ClickException(f"Invalid Terraform state format at '{gcs_uri}': expected a JSON object.")
outputs = state_data.get("outputs", {})| if key not in outputs: | ||
| from pathlib import Path | ||
|
|
||
| location_desc = tf_state_location or f"GCP project: '{project_id}' / instance: '{instance_name}'" if use_remote else f"'{Path.cwd()}'" |
There was a problem hiding this comment.
The inline conditional expression combined with or has tricky operator precedence (since if-else has lower precedence than or in Python, it evaluates as (tf_state_location or ...) if use_remote else ...). While correct, this is highly prone to readability issues and future bugs. Splitting this into a clear if/else block is much more maintainable.
if use_remote:
location_desc = tf_state_location or f"GCP project: '{project_id}' / instance: '{instance_name}'"
else:
location_desc = f"'{Path.cwd()}'"| assert result.exit_code == 0 | ||
| assert "VAL=gcs-resolved-val" in result.output | ||
| mock_client_inst.bucket.assert_called_once_with("tf-state-mock-instance-mock-project") | ||
| mock_bucket.blob.assert_called_once_with("terraform/state/mock-instance/default.tfstate") |
There was a problem hiding this comment.
Assert that the storage.Client is initialized with the correct project_id to ensure the client is explicitly bound to the target GCP project.
| assert result.exit_code == 0 | |
| assert "VAL=gcs-resolved-val" in result.output | |
| mock_client_inst.bucket.assert_called_once_with("tf-state-mock-instance-mock-project") | |
| mock_bucket.blob.assert_called_once_with("terraform/state/mock-instance/default.tfstate") | |
| assert result.exit_code == 0 | |
| assert "VAL=gcs-resolved-val" in result.output | |
| mock_storage_client.assert_called_once_with(project="mock-project") | |
| mock_client_inst.bucket.assert_called_once_with("tf-state-mock-instance-mock-project") | |
| mock_bucket.blob.assert_called_once_with("terraform/state/mock-instance/default.tfstate") |
| mock_client_inst.reset_mock() | ||
| mock_bucket.reset_mock() | ||
| result = runner.invoke( |
There was a problem hiding this comment.
Reset mock_storage_client before the second test case to prevent assertions from being affected by the first test case.
| mock_client_inst.reset_mock() | |
| mock_bucket.reset_mock() | |
| result = runner.invoke( | |
| mock_storage_client.reset_mock() | |
| mock_client_inst.reset_mock() | |
| mock_bucket.reset_mock() | |
| result = runner.invoke( |
| assert result.exit_code == 0 | ||
| assert "VAL=gcs-resolved-val" in result.output | ||
| mock_client_inst.bucket.assert_called_once_with("custom-bucket") | ||
| mock_bucket.blob.assert_called_once_with("custom-prefix/state.tfstate") |
There was a problem hiding this comment.
Assert that the storage.Client is initialized without arguments when only tf-state-location is provided.
| assert result.exit_code == 0 | |
| assert "VAL=gcs-resolved-val" in result.output | |
| mock_client_inst.bucket.assert_called_once_with("custom-bucket") | |
| mock_bucket.blob.assert_called_once_with("custom-prefix/state.tfstate") | |
| assert result.exit_code == 0 | |
| assert "VAL=gcs-resolved-val" in result.output | |
| mock_storage_client.assert_called_once_with() | |
| mock_client_inst.bucket.assert_called_once_with("custom-bucket") | |
| mock_bucket.blob.assert_called_once_with("custom-prefix/state.tfstate") |
| tfvars_content = tfvars_example | ||
| tfvars_content = tfvars_content.replace('"$$PROJECT_ID$$"', f'"{project_id}"') | ||
| tfvars_content = tfvars_content.replace('"$$NAMESPACE$$"', f'"{namespace}"') | ||
| tfvars_content = tfvars_content.replace('"$$NAMESPACE$$"', f'"{instance_name}"') |
There was a problem hiding this comment.
Any reason to leave the NAMESPACE here?
| """Initialize Terraform scaffolding for Data Commons administration/infrastructure.""" | ||
| click.secho("Data Commons Admin Init", fg="cyan", bold=True) | ||
|
|
||
| # For backward compatibility, fallback to namespace if instance_name is empty |
There was a problem hiding this comment.
I think we rip the bandaid off tbh. Or we ll carry this over far longer than we need.
No one is really using this yet, if so, they can continue using previous version of the CLI.
| project_id: str | None, | ||
| instance_name: str | None, | ||
| ) -> dict: | ||
| from google.cloud import storage |
There was a problem hiding this comment.
nit: move imports to top of file?
|
|
||
| if use_remote: | ||
| outputs = _get_outputs_from_gcs(tf_state_location, project_id, instance_name) | ||
| else: |
There was a problem hiding this comment.
can we move this elsewhere and put it in a helper for validation to use the CLI (or at least this function)?
There's no point in fetching the terraform state and doig all that logic above if it's gonna fail here.
| blob = bucket.blob(blob_name) | ||
|
|
||
| try: | ||
| content = blob.download_as_text() |
There was a problem hiding this comment.
one criticism from Jorge is that he doesn't want us to bring in the terraform state locally as it opens up the possibility of someone running a tf command they shouldnt.
For this specific scenario, we can just read the file from GCS and parse the output without copying the data over in the local terraform state right?
Adds support for fetching Terraform state outputs directly from GCS, allowing DCP admin CLI commands to be executed from anywhere outside of the local initialized Terraform folder.
Key Changes:
admincommand group:--project-id: GCP project ID used to locate the state bucket.--instance-name: DCP instance name used to locate the state bucket.--tf-state-location: Direct GCS URI pointing to the Terraform state file.get_terraform_outputto check Click context parameters. If specified, it fetches the state JSON from GCS and extracts values directly. If not, it falls back to localterraform output -json.Usage Examples:
Execute configuration tasks from outside your deployment directory:
(Note: Chained off 'deprecate-namespace' branch/PR)