-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix panic when environment uses computed values #414
Conversation
yamlBytes, err := getBytesFromAsset(inputMap["yaml"].AssetValue()) | ||
if err != nil { | ||
return nil, err | ||
yamlBytes := []byte{} |
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.
Hmm... does this mean there will be no yamlBytes
if it's computed? Or does this only affect Check
and the resolved value of environment.yaml is still correct?
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 only affects Check
. I have confirmed the resolved value is still correct (I'm assuming that it's ok to not have computed values during Check?)
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.
The purpose of this Check is to take the string value of the yaml and put that into state, so there's always a string value to compare against. In this case, when it's computed, it'll result in yaml being empty in state. So next time there's an update, there will be a diff because there is no value for yaml
. You can test this by adding an example test similar to in https://github.com/pulumi/pulumi-pulumiservice/pull/391/files
We'll need a way to get the string value even out of the computed property I think.
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.
Computed/unknown values will only appear during a preview, so I think that this will be okay. We'll re-run the Check
during the update, at which point all values will be known.
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.
Though I do think I'd do this slightly differently--instead of checking IsAsset
I'd check for !IsComputed
.
8195520
to
641e8cd
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.
can we add a test for this (do we have tests for the provider in here in general)?
641e8cd
to
c9f6912
Compare
Fixes #411
This is related to #399
Example repro program: