Skip to content

Conversation

nyobe
Copy link
Contributor

@nyobe nyobe commented Oct 8, 2025

Proposed changes

Describes how and why to use open approvals in ESC.

Unreleased product version (optional)

This feature is not yet released.

Related issues (optional)

Closes https://github.com/pulumi/pulumi-service/issues/31364

@nyobe nyobe requested a review from tehsis October 8, 2025 22:19
@nyobe nyobe requested a review from a team October 8, 2025 22:19
Copy link
Contributor

claude bot commented Oct 8, 2025

PR Review: Open Approvals Documentation

This PR adds comprehensive documentation for the open approvals feature in Pulumi ESC. Overall, the content is well-structured, clear, and follows most style guidelines. However, there are several issues that need to be addressed before merging.

Critical Issues

1. Filename capitalization inconsistency

  • The file is named Approvals.md (capital A), but Hugo content files should use lowercase with hyphens.
  • Fix: Rename the file to approvals.md using git mv to preserve history.

2. Incorrect heading capitalization (Lines 47 and 74)

  • Configuring a Ruleset should be Configuring a ruleset (sentence case for H3)
  • Fix: Change to sentence case per STYLE-GUIDE.md line 29.

3. Anchor link case mismatch (Line 76)

  • Links to #configuring-a-ruleset but the heading is Configuring a Ruleset which will generate #configuring-a-ruleset in Hugo.
  • This should work, but verify after fixing the heading capitalization.

Style Issues

4. Spacing inconsistency (Line 46 of access-control.md)

  • Trailing space at the end of the line.
  • Fix: Remove trailing space per AGENTS.md.

5. Inconsistent terminology (Lines 87-89)

  • Uses inconsistent formatting for field labels in the bullet list.
  • Consider using bold for Description, Access duration, and Grant expiration consistently.

Content Issues

6. Unrelated Go module changes

  • Multiple go.mod files were updated with version bumps (Go 1.23.0, toolchain 1.24.1, Pulumi SDK 3.175.0).
  • These changes appear unrelated to the open approvals documentation.
  • Question: Are these intentional or should they be in a separate PR?

7. Example clarity (Line 112)

  • The duration/expiration example is helpful but quite complex. Consider breaking it into smaller sentences or using a simpler scenario.
  • The sentence starting with If you first open it... is 42 words long, which can be difficult to parse.

Minor Suggestions

8. Navigation consistency (Line 85)

  • Navigate to the environment could be more specific: In the Pulumi Cloud console, navigate to the environment...

9. Missing newline

  • All files end with newlines correctly. ✓

10. Link validity

  • Internal link /docs/esc/administration/approvals#open-approvals looks correct.

Summary

The documentation is well-written and provides clear, actionable guidance for users. The main issues are:

  1. Filename capitalization (critical)
  2. Heading capitalization (style violation)
  3. Unrelated Go module changes (needs clarification)
  4. Minor style and clarity improvements

Please address the critical issues before merging. The content itself is excellent and will be a valuable addition to the ESC documentation.

@pulumi-bot
Copy link
Collaborator

@@ -0,0 +1,136 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

does filename casing matters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, but the claude bot pr review highlighted it as a CRITICAL ISSUE 😆 happy to revert tho

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@nyobe nyobe enabled auto-merge (squash) October 13, 2025 19:44
@pulumi-bot
Copy link
Collaborator

@nyobe nyobe merged commit 4fa8be6 into master Oct 13, 2025
8 checks passed
@nyobe nyobe deleted the claire/open-approval-docs branch October 13, 2025 19:51
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.

3 participants