Skip to content
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

Cleanup SetResourceInstanceCurrent to be clearer and more reliable #23475

Merged
merged 13 commits into from
Dec 4, 2019

Conversation

pselle
Copy link
Contributor

@pselle pselle commented Nov 22, 2019

Refactors setResourceInstanceCurrent which does lots of things, to do them in a more reliable and predictable order. Deletes dead code in eval_diff discovered over the course of investigation, following our "clean it up if you're nearby" practice.

@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Nov 22, 2019
@jbardin
Copy link
Member

jbardin commented Nov 22, 2019

Some of the test failures are typical, they were written to expect the wrong output.

The problematic ones are where we have the wrong provider stored in the state. I don't think this change is at fault for that however, and I bet it's highlighting that we don't update the provider consistently throughout the lifecycle of the resource.

@pselle
Copy link
Contributor Author

pselle commented Nov 22, 2019

@jbardin disagree, I think this change is at fault ... I added a line (where we call setMeta... again ...) that fixes them. Actually, changed it since it's only the provider update.

@pselle pselle marked this pull request as ready for review November 22, 2019 21:57
@jbardin
Copy link
Member

jbardin commented Nov 22, 2019

yeah, It could just be because the function is overloaded, but the fact that the wrong providers were in there make me wonder if it's not set consistently, and only cleaned up "just in time". (your last fix is still good, we want to restore everything this function needs to do for now)

@pselle pselle requested a review from a team November 22, 2019 22:22
@pselle pselle requested review from a team and removed request for a team December 2, 2019 16:12
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This looks generally good! I have one little concern inline regarding a condition I saw in the old implementation of this function which didn't seem to be covered in the new.

states/module.go Show resolved Hide resolved
@pselle pselle force-pushed the pselle/setMetaCleanup branch from 6e909e3 to de953ec Compare December 3, 2019 21:29
@pselle
Copy link
Contributor Author

pselle commented Dec 3, 2019

@apparentlymart Very very helpful catch, found and fixed.

// check for an existing resource, now that we've ensured that rs.Instances is more than 0/not nil
is := rs.Instance(addr.Key)
if is == nil {
// if there is no instance, but the resource exists and has other instances,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is now not totally accurate, because we can get here in the case where the resource exists but has no other instances. The code around it seems correct, but since you've gone to the effort of adding all of these great comments I thought you'd probably want to clarify this one. 😀

@pselle pselle merged commit 2a2201c into master Dec 4, 2019
@ghost
Copy link

ghost commented Mar 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
3 participants