-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
stacks: handle deferred actions in refresh #34887
Conversation
"upstream_names": cty.NullVal(cty.Set(cty.String)), | ||
}), | ||
}, | ||
wantDeferred: make(map[string]providers.DeferredReason), |
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'd expect to see a value in here, if the resource is being deferred. Do we know why there isn't anything?
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.
Yeah, looked into it, seems like my mock was incorrect and I was testing the not-deferred path all along. Since I was a bit confused what I should be asserting mid way through this issue I was assuming the test to be right and relying on you to verify my assertions to be correct. Now that I fixed the mock it works (and I found a panic to fix, double win)
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.
LGTM, just some minor nits wrt debugging statements and comments.
b3eac2b
to
bdf223b
Compare
We are moving the deferred reason from plans/deferring to providers to have a single representation of deferred reason to be used from providers to the plan. Using the plan's representation causes circular dependencies. Co-authored-by: Matej Risek <[email protected]>
Co-authored-by: Matej Risek <[email protected]>
before marking it as deferred
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Co-authored-by: Matej Risek [email protected]
Target Release
1.8.x
Draft CHANGELOG entry
NEW FEATURES