Skip to content

Conversation

@Frassle
Copy link
Member

@Frassle Frassle commented Nov 20, 2025

Proposed changes

Unreleased product version (optional)

Related issues (optional)

@claude
Copy link
Contributor

claude bot commented Nov 20, 2025

Documentation Review

I've reviewed the new Stash resource documentation. Overall, this is a well-structured introduction to the Stash feature with clear examples across all supported languages. Here are my findings:

Issues Found

Grammar and Clarity

Line 16: Missing article before "built-in"

  • Current: "The Stash resource is a built-in Pulumi resource"
  • Suggestion: Consider "The Stash resource is a built-in Pulumi resource" (already correct, no change needed)

Line 17: Awkward phrasing - "This value is stateful and won't change"

  • The phrase "won't change on later deployments if input is changed" is confusing. It seems contradictory.
  • Suggest rewording to clarify that the output property preserves the original value even when input changes:
Every `Stash` resource accepts any value as its `input` property and makes that value available through its `output` property. The `output` value is stateful and persists the original `input` value even when the `input` property is updated in subsequent deployments. The current `input` value is always available as an output property if you need to reference the updated value.

Line 132: Grammar error - "it's" should be "its"

  • Current: "Stash is like any other resource in that it's name must be unique"
  • Should be: "Stash is like any other resource in that its name must be unique"

Style Guide Violations

Line 345: The update behavior section is unclear about the core concept

  • The section "Updating stashed values" needs clearer explanation of the distinction between input and output behavior.
  • Consider restructuring this section to first clearly explain that output is immutable (preserves the original value) and input is mutable (can be updated).

Line 348-352: List formatting issue

  • The three methods for replacing the stash should be formatted as a proper ordered list per STYLE-GUIDE.md:
To update the `output` value the `Stash` resource needs to be replaced. There are a few ways to do this:

1. Using the `--target-replace` argument to `up` to tell the engine to replace it.
1. Using `pulumi state taint` to mark the resource to be replaced on the next deployment.
1. Using the `TriggerReplacement` resource option to trigger the resource to replace on a change of value.

Code Examples

Lines 382, 405, 541, 575: Go example has incorrect username retrieval

  • The Go example uses os.UserHomeDir() to get the username, but this returns the home directory path, not the username.
  • Should use os.Getenv("USER") or user.Current().Username:
import (
    "os/user"
    "github.com/pulumi/pulumi/sdk/v3/go/pulumi"
)

func main() {
    pulumi.Run(func(ctx *pulumi.Context) error {
        currentUser, err := user.Current()
        if err != nil {
            return err
        }

        firstDeployer, err := pulumi.NewStash(ctx, "firstDeployer", &pulumi.StashArgs{
            Input: pulumi.String(currentUser.Username),
        })
        if err != nil {
            return err
        }

        // The output will always show the original deployer, even if others run updates later
        ctx.Export("originalDeployer", firstDeployer.Output)
        return nil
    })
}

Lines 551, 569, 599, 625, 648: Incomplete examples with placeholder function calls

  • Multiple examples reference undefined functions like generatePassword() without showing the implementation or import.
  • These examples won't run as-is. Either show the complete implementation or use inline password generation:

For TypeScript (line 551):

import * as pulumi from "@pulumi/pulumi";
import * as random from "@pulumi/random";

// Generate a random password once
const randomPassword = new random.RandomPassword("dbPassword", {
    length: 16,
    special: true,
});

// Stash it so it doesn't change on subsequent deployments
const passwordStash = new pulumi.Stash("passwordStash", {
    input: pulumi.secret(randomPassword.result),
});

// Use the stashed password for database configuration
export const dbPassword = passwordStash.output;

Similar fixes needed for Python (line 569), Go (line 599), C# (line 625), and Java (line 648).

Missing Content

Line 706: Missing trailing newline

  • The file should end with a newline character per STYLE-GUIDE.md and AGENTS.md.

Summary

This documentation provides a solid foundation for understanding the Stash resource. The main issues are:

  • Grammar error (it's vs its)
  • Unclear explanation of the input/output update behavior
  • Go code examples with incorrect username retrieval
  • Incomplete code examples with undefined function calls
  • Missing trailing newline

Once these issues are addressed, this will be excellent documentation for the Stash feature.


Mention @claude if you'd like me to review the updated changes or help fix any of these issues.

@pulumi-bot
Copy link
Collaborator

@Frassle
Copy link
Member Author

Frassle commented Nov 20, 2025

@claude review this again

@claude
Copy link
Contributor

claude bot commented Nov 20, 2025

Claude finished @Frassle's task —— View job


Re-review of Stash Documentation

Todo List

  • Read current version of stash.md
  • Compare against previous review feedback
  • Identify remaining and new issues
  • Provide comprehensive review feedback

Review Findings

I've reviewed the current version of the documentation. Here's what I found:

✅ Issues Fixed

Great work! The following issues from my previous review have been addressed:

  1. Line 18 - The awkward phrasing about "won't change" has been improved significantly. The explanation is now much clearer.
  2. Line 136 - The grammar error ("it's" → "its") has been fixed correctly.
  3. Lines 340-342 - The update methods are now properly formatted as an ordered list.
  4. Line 402 - The Go example for capturing the first deployment user has been fixed to use os.Getenv("USER") instead of os.UserHomeDir().
  5. Line 702 - The file now ends with a newline character.

⚠️ Remaining Issues

Lines 583, 602, 624, 654, 685 - Incomplete examples with undefined generatePassword() function

The examples in the "Preserving a stable random value" section still reference an undefined generatePassword() function. These examples won't work as written.

Why this matters: Users copying these examples will get compilation/runtime errors. The examples should either:

  1. Show the complete implementation of generatePassword(), or
  2. Use inline random password generation instead of calling an undefined function

Suggested fix - Replace with working examples like:

TypeScript (line 583):

import * as pulumi from "@pulumi/pulumi";
import * as random from "@pulumi/random";

// Generate a random password once
const randomPassword = new random.RandomPassword("dbPassword", {
    length: 16,
    special: true,
});

// Stash it so it doesn't change on subsequent deployments
const passwordStash = new pulumi.Stash("passwordStash", {
    input: pulumi.secret(randomPassword.result),
});

// Use the stashed password for database configuration
export const dbPassword = passwordStash.output;

Similar fixes needed for Python (line 602), Go (line 624), C# (line 654), and Java (line 685).

📋 Minor Observation

Line 344-346 - The explanation of the update behavior could be slightly clearer:

  • Current: "Without a replacement any changes to the input input will be reflected in the output property input, but the output property output will not change."
  • The phrase "output property input" is a bit confusing since input is typically thought of as an input, not an output property.
  • Consider: "Without a replacement, any changes to input will be reflected in the input output property, but the output property will not change and will continue to return the original value."

Summary

The documentation has been significantly improved since the last review! Most of the critical issues have been fixed:

  • ✅ Grammar and style issues resolved
  • ✅ Code example correctness (Go username retrieval) fixed
  • ✅ List formatting corrected
  • ✅ File formatting (trailing newline) fixed

One remaining issue: The "Preserving a stable random value" examples still call an undefined generatePassword() function across all 5 languages. This needs to be fixed before merge to avoid confusing users with non-working examples.

Once the generatePassword() examples are fixed, this documentation will be ready for publication!


|

Copy link
Contributor

@julienp julienp left a comment

Choose a reason for hiding this comment

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

LGTM! We can come back here and link to TriggerReplacement when we have the docs for that shipped. I think there might still be some of the SDKs outstanding for that?


## Stashing secret values

The `Stash` resource respects secret annotations. If the `input` value is marked as a secret, the `output` will also be secret, and the value will be encrypted in your stack's state.
Copy link
Member

Choose a reason for hiding this comment

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

just curious--did we validate that this holds for nested inputs?

Copy link
Member Author

Choose a reason for hiding this comment

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

We roundtrip the property value as-is, so it will depend on the language marshaling layer but Stash never reduces the amount of secretness


{{< /chooser >}}

When viewing stashed secret values, their plaintext content will not be shown by default. Instead, they will be displayed as `[secret]` in the CLI. Pass `--show-secrets` to reveal the plaintext value.
Copy link
Member

Choose a reason for hiding this comment

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

pass --show-secrets where? up/preview/etc.?

Copy link
Member Author

Choose a reason for hiding this comment

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

up, preview, stack, export, there's a load of commands that take this option. Maybe
"Pass --show-secrets to the command ran to reveal the plaintext value."?

Copy link
Member

Choose a reason for hiding this comment

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

yeah that sounds reasonable

Comment on lines +340 to +342
1. Using the `--target-replace` argument to `up` to tell the engine to replace it.
1. Using `pulumi state taint` to mark the resource to be replaced on the next deployment.
1. Using the `TriggerReplacement` resource option to trigger the resource to replace on a change of value.
Copy link
Member

Choose a reason for hiding this comment

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

I'd reorder these so the resource option is first, personally.

also wondering if we should consider a keeper-style property in the future that has replace semantics

Copy link
Member Author

Choose a reason for hiding this comment

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

also wondering if we should consider a keeper-style property in the future that has replace semantics

Briefly what would that look like? I'd have thought that's what TriggerReplacement is.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I think I misunderstood that bullet point. Yeah we can just use that.

Though that does raise another question--where are the docs for that option? I don't see it here: https://www.pulumi.com/docs/iac/concepts/resources/options/

Kind of a meta-point that it would be great to add links to the docs for the various suggested commands as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

TomH is still getting around to finishing that workstream off. I was planning on linking it once it was in place.

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.

5 participants