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

context Refresh and Apply sometimes return nil #13665

Merged
merged 1 commit into from
Apr 14, 2017

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Apr 14, 2017

The documentation for Refresh indicates that it will always return a
valid state, but that wasn't true in the case of a graph builder error.
While this same concept wasn't documented for Apply, it was still
assumed in the terraform apply code.

Since the helper testing framework relies on the absence of a state to
determine if it can call Destroy, the Context can't can't start
returning a state in all cases. Document this, and use the State method
to fetch the correct state value after Apply.

Add a nil check to the WriteState function, so that writing a nil state
is a noop.

Make sure to init before sorting the state, to make sure we're not
attempting to sort nil values. This isn't technically needed with the
current code, but it's just safer in general.

Fixes #13565

@apparentlymart
Copy link
Contributor

It looks like a couple tests are failing with a nil pointer dereference error here, though not enough context in the Travis log for me to guess whether it was actually caused by this change. Hard to imagine how this change would cause it unless there are cases where these methods are called on a nil Context...

@jbardin
Copy link
Member Author

jbardin commented Apr 14, 2017

@apparentlymart: interesting. I'll take a look locally - I admit I delegated testing this to travis ;)

@jbardin jbardin force-pushed the jbardin/context-return-state branch 2 times, most recently from 8aa8069 to f964d50 Compare April 14, 2017 18:45
@jbardin jbardin changed the title context Refresh and Apply must always return state context Refresh and Apply sometimes return nil Apr 14, 2017
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.

Looks like in the long run there's some cleanup worth doing here to limit how much these nil states get passed around, but this seems like a fine short-term change to keep us rolling!

The documentation for Refresh indicates that it will always return a
valid state, but that wasn't true in the case of a graph builder error.
While this same concept wasn't documented for Apply, it was still
assumed in the terraform apply code.

Since the helper testing framework relies on the absence of a state to
determine if it can call Destroy, the Context can't can't start
returning a state in all cases. Document this, and use the State method
to fetch the correct state value after Apply.

Add a nil check to the WriteState function, so that writing a nil state
is a noop.

Make sure to init before sorting the state, to make sure we're not
attempting to sort nil values. This isn't technically needed with the
current code, but it's just safer in general.
@jbardin jbardin force-pushed the jbardin/context-return-state branch from f964d50 to 928e606 Compare April 14, 2017 18:56
@jbardin jbardin merged commit 1771ba1 into master Apr 14, 2017
@jbardin jbardin deleted the jbardin/context-return-state branch April 14, 2017 18:56
@ghost
Copy link

ghost commented Apr 14, 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 Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.9.2 panic instead of cycle error
2 participants