-
Notifications
You must be signed in to change notification settings - Fork 1
WIP: try to bubble up resource replacements to module replacements #425
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: main
Are you sure you want to change the base?
Conversation
This change is part of the following stack: Change managed by git-spice. |
Relates to #397 |
That's a good guess. Trying this out. Also want to try replaceOnChanges with one resource. |
replaceOnChanges does not in fact work. I suspect it is pulumi/pulumi#19972 |
inputsChanged := false | ||
if !oldInputs.DeepEquals(newInputs) { | ||
// Inputs have changed, so we need tell the engine that an update is needed. | ||
return &pulumirpc.DiffResponse{Changes: pulumirpc.DiffResponse_DIFF_SOME}, nil |
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 looks suspect as it's tricky to reason about short-circuit here, thinking.
ctx, | ||
urn, | ||
oldInputs, | ||
newInputs, |
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 looks suspect! Why the change.
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.
Ah. @Zaid-Ajaj explains that before the change oldInputs.DeepEquals(newInputs) made them the same.
} | ||
if len(replaceKeys) > 0 { | ||
resp.Replaces = replaceKeys | ||
resp.DeleteBeforeReplace = true |
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.
DeleteBeforeReplace gets past the error! Promising!
The test failures are around the new behavior, we could adjust these to accept the new behavior. I am leaning to rejecting this though and leaving as won't fix / using documentation. |
Exploring how to lift resource replaces planned by TF into module replacements.
This code currently successfully causes the module to replace, but errors during the update:
My best guess is that we failed to indicate to the engine that the resources within the module would also be replaced as a result of the module replacement?