-
Notifications
You must be signed in to change notification settings - Fork 256
Update ignoreChanges documentation #16161
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?
Conversation
Pull Request ReviewThis PR adds helpful documentation about how ignoreChanges works and provides a practical example. The content is generally well-written and clear. However, there are several issues that need to be addressed: Critical Issues1. Missing ignoreChanges in the example (Lines 112-403) The example titled "Example: Preserve externally managed weights" does not actually demonstrate using ignoreChanges. The code creates AWS resources but never shows the ignoreChanges option being applied to preserve the weights. This makes the example incomplete and confusing. You need to add the ignoreChanges option to the listener resource in all language examples. 2. Incomplete code examples (Lines 124-148, etc.) The load balancer and target group resources are created with empty configuration objects. These resources require multiple required arguments (VPC ID, subnets, health checks, etc.) and will not run as shown. The example should either include the minimum required configuration to make the code runnable, or add a comment indicating that configuration has been omitted for brevity. Style and Documentation Issues3. Shortcode syntax inconsistency (Line 29) The notes shortcode uses {{% notes "warning" %}} but according to the style guide (STYLE-GUIDE.md:49-50), the correct syntax should use type=. Apply this same fix to lines 405, 409, and 415. 4. Clarity issue (Line 27) The sentence "an external system can safely update the live resource as long as you synchronize the stack before the next update" is potentially confusing. Consider rewording to: "an external system can update the live resource, but you must synchronize the stack before the next update" 5. Paragraph length (Lines 29-31) The warning note contains a single long sentence. Consider breaking it into 2-3 shorter sentences for better readability per STYLE-GUIDE.md:69. 6. .gitignore change (Lines 72-74) The addition of mise.local.toml to .gitignore appears unrelated to this documentation update. Unless there is a specific reason this belongs in this PR, consider removing it or explaining the connection. Minor Issues7. Passive voice (Line 110) "This is common after you have imported" could be reworded to avoid passive construction. Consider: "This commonly occurs after importing existing infrastructure..." 8. Property path documentation (Line 403) The example states to run pulumi refresh before adding a third target group, but does not explain what the correct property path would be for ignoreChanges to preserve the weights. Consider adding this detail. Recommendations
The core content and explanation of how ignoreChanges works (lines 19-31) is excellent and addresses a real user pain point. Once the example code is corrected and completed, this will be a valuable addition to the documentation. |
Your site preview for commit ae92d76 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16161-ae92d763.s3-website.us-west-2.amazonaws.com. |
Your site preview for commit 8cf5173 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16161-8cf51733.s3-website.us-west-2.amazonaws.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a little bit of missing continuity and context (I left comments), but overall this looks good.
{{% notes type="warning" %}} | ||
When you skip `pulumi refresh` (or `pulumi up --refresh`) after `ignoreChanges` has been set, Pulumi keeps using the previous state value when it performs an update. Providers that require full object replacements—such as AWS load balancer listeners where the entire target group array is sent on every update—will receive the stale values from the state and may reset the live configuration. | ||
{{% /notes %}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true if and only if you are using ignoreChanges
to allow non-IaC management of a field. I don't think its fair to assume that all uses of ignoreChanges
expect external management (and thus need a refresh before update).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add something here to note that, but it's not that you necessarily expect external management. Your cloud state could have drifted unintentionally and the result will be the same. The only way to guarantee that you are ignoring changes is if you refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tldr. Good point. LGTM
Consider the following program + state + cloud:
State:
{
"oldInputs": {
"x": "a"
}
}
Program:
new example.Resource_X("example", {
x: "b"
}, { ignoreChanges: [ "x" ] });
Cloud (as returned by Read
):
{
"x": "c"
}
You have added ignoreChanges: [ "x" ]
, so we definitely don't want the result of running anything to set state or the cloud to "b"
.
If you want Resource_X.x
to be cloud managed, then you want to change the state to have { "x": "c" }
and you should run a refresh.
If you want Resource_X.x
to ramain "a"
(since that is the last program input)... you can't ever run refresh. This isn't really supported <-- at this point I figured out I was wrong about how refresh interacted with ignoreChanges
Your site preview for commit 82c0d12 is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16161-82c0d122.s3-website.us-west-2.amazonaws.com. |
Your site preview for commit fa5b24a is ready! 🎉 http://www-testing-pulumi-docs-origin-pr-16161-fa5b24a1.s3-website.us-west-2.amazonaws.com. |
{{% notes type="warning" %}} | ||
When you skip `pulumi refresh` (or `pulumi up --refresh`) after `ignoreChanges` has been set, Pulumi keeps using the previous state value when it performs an update. Providers that require full object replacements—such as AWS load balancer listeners where the entire target group array is sent on every update—will receive the stale values from the state and may reset the live configuration. | ||
{{% /notes %}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tldr. Good point. LGTM
Consider the following program + state + cloud:
State:
{
"oldInputs": {
"x": "a"
}
}
Program:
new example.Resource_X("example", {
x: "b"
}, { ignoreChanges: [ "x" ] });
Cloud (as returned by Read
):
{
"x": "c"
}
You have added ignoreChanges: [ "x" ]
, so we definitely don't want the result of running anything to set state or the cloud to "b"
.
If you want Resource_X.x
to be cloud managed, then you want to change the state to have { "x": "c" }
and you should run a refresh.
If you want Resource_X.x
to ramain "a"
(since that is the last program input)... you can't ever run refresh. This isn't really supported <-- at this point I figured out I was wrong about how refresh interacted with ignoreChanges
Proposed changes
Updating the docs on
ignoreChanges
to make it clearer to users some of the current limitations.See the internal doc for motivation.
Link to the specific page http://www-testing-pulumi-docs-origin-pr-16161-8cf51733.s3-website.us-west-2.amazonaws.com/docs/iac/concepts/options/ignorechanges/
Unreleased product version (optional)
Related issues (optional)