Skip to content

Conversation

@nyobe
Copy link
Contributor

@nyobe nyobe commented Oct 30, 2025

Proposed changes

The esc terraform-state provider enables importing outputs from a terraform state file into an environment.
This is the equivalent of the pulumi-stacks provider for terraform.

Related issues (optional)

@nyobe nyobe requested a review from a team October 30, 2025 02:46
@claude
Copy link
Contributor

claude bot commented Oct 30, 2025

Documentation Review

Thank you for adding the terraform-state provider documentation! Overall, this is well-structured and follows the repository conventions. I found a few minor issues that should be addressed:

Issues Found

1. Missing Newline at End of File (Line 95)

The file content/docs/esc/integrations/infrastructure/terraform/terraform-state.md does not end with a newline character. According to AGENTS.md, all files must end with a newline.

Fix: Add a newline at the end of the file.

2. Table Formatting: Optional Properties Not Marked (Lines 80-84)

In the "Remote" backend table, the hostname property is optional (as mentioned in the text below the table), but this is not indicated in the table itself. For consistency with other provider docs (e.g., aws-login.md), optional properties should be marked as [Optional] - in the Description column.

Suggestion:

| Property       | Type   | Description                                                 |
|----------------|--------|-------------------------------------------------------------|
| `hostname`     | string | [Optional] - The hostname of Terraform Cloud/Enterprise. Defaults to `app.terraform.io`. |
| `organization` | string | The Terraform Cloud organization name.                     |
| `workspace`    | string | The workspace name.                                         |
| `token`        | string | The Terraform Cloud API token.                             |

Then you can remove the separate note: "The hostname property defaults to app.terraform.io if not specified."

3. Consistency: "Terraform Cloud/Enterprise" vs "Terraform Cloud" (Line 81)

The hostname description mentions "Terraform Cloud/Enterprise" but the organization and workspace descriptions only mention "Terraform Cloud". For consistency, either use "Terraform Cloud/Enterprise" throughout or clarify which properties apply to both.

Positive Observations

✓ Proper use of aliases for SEO preservation
✓ Correct heading capitalization (H1 in frontmatter, sentence case for H2+)
✓ Consistent table formatting with other provider docs
✓ Clear examples for both S3 and Terraform Cloud backends
✓ Proper menu structure with parent identifier
✓ Link to aws-login documentation is valid

Action Required

Please address the three issues above, particularly #1 (missing newline) which is a strict requirement per AGENTS.md.


If you'd like me to review the changes after you've made updates, or if you need help implementing any of these suggestions, mention me (@claude) in a comment.

@pulumi-bot
Copy link
Collaborator

@nyobe nyobe marked this pull request as ready for review October 30, 2025 07:34
@claude
Copy link
Contributor

claude bot commented Oct 30, 2025

Documentation Review

Thank you for adding documentation for the ESC terraform-state provider! This is a well-structured addition. I found a few items to address:

Issues Found

terraform-state.md:82 - Inconsistent optional formatting

  • The [Optional] prefix should follow the same pattern as other ESC provider docs. In the aws-login provider (and others), the format is [Optional] - with a space after the closing bracket and before the dash.
  • Current: [Optional] - The remote backend hostname...
  • This is actually correct and consistent. No change needed.

terraform-state.md:19 - Section heading case

  • The heading "Examples" on line 19 should be singular to match the pattern in other provider docs (e.g., aws-login uses "Example" on line 19).
## Example

terraform-state.md:1-13 - Missing h1 field

  • The frontmatter includes both title and h1 fields set to "terraform-state", but reviewing other provider docs like aws-login.md, this is actually consistent. However, the title field comes before h1 in aws-login, but the order is reversed here. For consistency:
---
title_tag: terraform-state Pulumi ESC Provider
meta_desc: The terraform-state provider enables you to read outputs from Terraform state files stored in S3 or Terraform Cloud.
title: terraform-state
h1: terraform-state
menu:
  esc:
    identifier: terraform-state
    parent: esc-terraform-integrations
    weight: 1
aliases:
  - /docs/pulumi-cloud/esc/providers/terraform-state/
  - /docs/esc/providers/terraform-state/
---

terraform-state.md:93 - Missing trailing newline

  • The file should end with a newline character after line 93 per repository standards (AGENTS.md specifies "Must always end with a newline").

Positive Notes

  • The menu structure with parent: esc-terraform-integrations correctly references the identifier added to the terraform/_index.md file
  • The aliases are properly configured to handle old URL paths
  • The documentation structure (Examples, Inputs, Outputs) follows the established pattern
  • The YAML examples are clear and well-formatted
  • Table formatting is clean and consistent
  • Cross-reference link to aws-login documentation is correct

Recommendations

  1. Change "Examples" to "Example" (singular) for consistency
  2. Reorder frontmatter fields to match established pattern (title before h1)
  3. Ensure file ends with a trailing newline

Once you've addressed these items, feel free to mention me (@claude) if you'd like another review or have questions!

@nyobe nyobe enabled auto-merge (squash) October 31, 2025 08:34
Copy link
Member

@fnune fnune left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Fausto Núñez Alberro <fausto.nunez@mailbox.org>
@pulumi-bot
Copy link
Collaborator

@nyobe nyobe merged commit ee2e1ab into master Oct 31, 2025
8 checks passed
@nyobe nyobe deleted the claire/tf-state branch October 31, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants