Skip unknowns in Check to avoid panic when marshalling Build object

[guineveresaenger]

When we try to access a Build object which contains unknown values in the Check() function, the provider panics, because the Build object is not of the correct resource type.
This pull request ignores unknowns when reading inputs in Check(), allowing for the resulting object to be processed correctly.
Fixes #565.

[github-actions]

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

[mikhailshilkov]

Is there a test we could add for this case?

[iwahbe]

What will line 121 do if build is an unknown?

On line 179 we check if input["build"] is null. Will that work (not panic)? Will that do the wrong thing because inputs is actually unknown, not null?

[t0yv0]

It's good to fix the bug in any way we can for now! (if we can add a test). Thinking forward, is there a chance that this is actually a platform issue? As I've attempted in #435 it feels like there should be a way to code against a higher-level interface that hydrates a Go struct with appropriate err msgs, and concerns such as input["build"] panicking that Ian brings up would just not arise in that repr.

[guineveresaenger]

All good questions!

@mikhailshilkov I am completely open to suggestion here, but as this is SDK behavior, testing it here feels redundant at best and incorrect at worst.

@iwahbe - if we set KeepUnknowns to false, Check will throw away any unknowns. This means that Build will never be an unknown. I looked at a few other Native providers (azure and k8s), and any IsUnknown() checks are quite rare throughout. As for line 121, it may be worth it to add an IsNull() check.

@t0yv0 - I chatted with @Frassle about it this morning, and in the panic situation, the Build object becomes flattened during Check(), meaning if any one sub-field is Unknown, the entire object is Unknown. This does mean we lose access to any fields that were set! However, for this use case, we don't actually need access to those fields since we're setting defaults.

[mikhailshilkov]

@guineveresaenger #565 has a repro so a baseline test could be exercising that repro in one of our end-to-end tests. I don't know if there is a lower-level test that would still make sense. Unit test Check and pass some unknowns?

[guineveresaenger]

@mikhailshilkov - ah, an integration test makes a lot more sense to me. Added!

[github-actions]

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

[iwahbe]

@iwahbe - if we set KeepUnknowns to false, Check will throw away any unknowns. This means that Build will never be an unknown. I looked at a few other Native providers (azure and k8s), and any IsUnknown() checks are quite rare throughout. As for line 121, it may be worth it to add an IsNull() check.

I meant "What will this do if build is genuinely unknown. I understand the provider won't get a resource.PropertyValue (since we discard unknowns). I meant will we handle a nil value correctly everywhere. I believe that is what a discard unknown turns into.

[guineveresaenger]

@iwahbe - gotcha! This doesn't show up because it's in a different file, but IsNull is the first thing the Build marshaller checks: https://github.com/pulumi/pulumi-docker/blob/master/provider/image.go#L361 so I think we should be good here!

[jazzyfresh]

Nice test!