-
Notifications
You must be signed in to change notification settings - Fork 256
esc: external provider #16589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
esc: external provider #16589
Conversation
Documentation ReviewI've reviewed PR #16589 which adds documentation for the external provider and rotator. Here are my findings: Issues Found1. Missing trailing newlines (both new files)Both new files are missing trailing newlines, which is required per AGENTS.md. Files affected:
Fix: Add a newline at the end of both files. 2. Table formatting inconsistencies in index filesFile: The table has inconsistent trailing whitespace. Line 25 is missing trailing whitespace after the description, while other rows have it. File: Same issue on line 27. File: Missing closing parenthesis in the external rotator link. 3. Inconsistent cross-reference link formatFile: The link format uses a non-standard relative path with README.md. Hugo content files should use standard path references. File: Same issue - use a proper anchor link to the dynamic-secrets external provider page. 4. Spelling/GrammarFile: Missing period at the end of the sentence. 5. Heading capitalizationFile: Heading "Recommended: Dual-Secret Strategy" should use sentence case per STYLE-GUIDE.md (H2+ headings use sentence case). Positive Observations
Recommendations
Please mention me (@claude) if you would like me to review the changes again after addressing these issues or if you need help with any fixes. |
56ebb33 to
c518d74
Compare
|
Your site preview for commit a593b3a is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16589-a593b3ac.s3-website.us-west-2.amazonaws.com. |
Co-authored-by: Boris Schlosser <boris@pulumi.com>
Documentation ReviewI've reviewed PR #16589 which adds documentation for the ESC external provider and rotator. Overall, the documentation is comprehensive and well-structured. Below are my findings: Issues Found1. Missing space in table (dynamic-secrets/_index.md:17)Line 17 in The description says "import Secrets" but should say "import secrets" (lowercase 's' for consistency with other entries in the table). 2. Inconsistent capitalization of "secrets" in table descriptionsThroughout both index files, there's inconsistent capitalization of "secrets":
Per the style guide, lowercase should be used for non-proper nouns ("stack" not "Stack"). The word "Secrets" is being capitalized unnecessarily. Note: The existing entries already have this issue - lines 17, 19-24 all capitalize "Secrets" inconsistently. While fixing the new external provider entry, consider fixing the existing entries as well for consistency. 3. Code comment style (external.md:167, 245, 295)The Python code examples include comments. Per AGENTS.md: "TypeScript/JavaScript: Must follow tsconfig.json settings. No comments unless explicitly requested." While this rule specifically mentions TypeScript/JavaScript, the principle applies to example code. The comments in the Python examples are actually helpful for understanding the code, so this is acceptable. However, consider whether all comments add value:
These comments are valuable, so no change needed. 4. Spelling: "adapter" vs "adaptor" (rotated-secrets/_index.md:20)Line 20 uses "adapter" consistently, which is correct. No issue here. Style Observations (No Changes Required)
Content QualityStrengths:
Suggestions for improvement:
SummaryThe documentation is well-written and comprehensive. The main issue is inconsistent capitalization of "Secrets" vs "secrets" in the table descriptions. Consider fixing this across both the new external provider entry and the existing entries for consistency. Great work on documenting this new feature! The authentication and state management sections are particularly well done. If you'd like me to review any changes or provide additional feedback, mention @claude in a comment. |
|
Your site preview for commit 48c1914 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16589-48c19146.s3-website.us-west-2.amazonaws.com. |
Proposed changes
Add docs for the external providers
Unreleased product version (optional)
To release with https://github.com/pulumi/pulumi-service/issues/33114
Currently under feature flag: https://app.launchdarkly.com/projects/default/flags/34661-esc-external-provider
Related issues (optional)
Implementation: https://github.com/pulumi/pulumi-service/pull/34661