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
[engine] Clear pending operations with refresh. #8435
Conversation
FWIW, this aspect of the implementation makes me a little uncomfortable. Another approach that I think is worth considering is something like this:
That approach feels a little more well-founded, as it only drops information re: the pending operation once we've explicitly taken action to resolve the operation's results. That said, taking this PR does not prevent us from adopting the "stronger" approach in the future. |
Before diving in implementation options, can I get a bit more context. Sounds like the user is running into interrupted updates and using our IUR procedure I was just looking up:
Is the idea that after the change the IUR becomes simply |
I'm reading the #4265 and it's helpful. Luke's comments constrain the scope of the fix. I need to do some more work here and take it for a spin to understand the changes. I like the "Another approach that I think is worth considering is something like this" as it's spelled out what's happening and I feel like I understand it. I feel like I need to understand and step through the approach in the PR. Open questions (notes to self to verify):
|
Not as written. This would probably be easy to do.
Yes, they are still blocked on pending operations.
This is the thrust of this PR. With these changes, the IUR procedure becomes "run |
Just what it says on the tin. This is implemented by moving the check for pending operations in the last statefile into the deployment executor and making it conditional on whether or not a refresh is being performed (either via `pulumi refresh` or `pulumi up -r`). Because pending operations are not carried over from the base statefile, this has the effect of clearing pending operations if a refresh is performed. Fixes #4265.
a3060a1
to
88f9ca4
Compare
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.
Changes and the test looking good!
I just really think we should send warnings when --refresh ignores maybe orphaned resources.
I tried adding this but printPendingOperationsError seems tied to Result which seems tied to early termination (in PR or quick follow up). Which API can we use to send warnings into the console for the user? Thanks.
I do not think I would consider #4265 resolved until we make this change. That is the key point of extreme friction in the current model. If we want to land this refresh change first, that sounds reasonable, but I think we should leave #4265 open to track addressing the core user experience pain here. |
I agree with @lukehoban, Pulumi should handle the |
As discussed with @Frassle I've made a change to this PR where pending CREATE operations are not cleared upon a refresh and these will require user intervention to resolve potentially orphaned resources. |
Just what it says on the tin. This is implemented by moving the check
for pending operations in the last statefile into the deployment
executor and making it conditional on whether or not a refresh is being
performed (either via
pulumi refresh
orpulumi up -r
). Becausepending operations are not carried over from the base statefile, this
has the effect of clearing pending operations if a refresh is performed.
Part of #4265.