Encode default values

[iwahbe]

Fixes pulumi/pulumi-aws#2646

This PR makes two distinct changes:

• cd9ad00: adds an explicit check for accepting a bool that should be a string, changing the return value to "true" and "false" instead of true and false.
• 796df14: calls MakeTerraformInput on values drawn from SchemaInfo.DefaultInfo.Value. This uses the machinery introduced in the previous commit.

WIP: Still needs tests for both features.

[github-actions]

Diff for pulumi-azuread with merge commit ebf2220

[github-actions]

Diff for pulumi-azuread with merge commit 09fe9f9

[t0yv0]

Well this is a massive transformation introduced here! We need to be careful. We're losing track of what the actual type is of defaultValue because it all collapses to any. There's two options it's either "any" representation of pulumi.PropertyValue or it's the "any" representation of TF value (hcty.Value).

[iwahbe]

Your right. This transformation should have been applied earlier. Fixed in 796df14.

[t0yv0]

Eh! I'm not sure! That depends on what we expect users to put in defaultValue. Just need to be careful if we're changing it need to call it out... change management..

[iwahbe]

We stick Default.Value in the pulumi schema, which is then bundled into our SDKs. It must match the pulumi type or we get compile errors.

If the pulumi value is the same as the TF value, then MakeTerraformInput won't do anything. If it isn't then the default value will be of the pulumi kind and we need the conversion.

[github-actions]

Diff for pulumi-azuread with merge commit 3f8fa20

[github-actions]

Diff for pulumi-azuread with merge commit c12bbf8

[github-actions]

Diff for pulumi-azuread with merge commit 24b1045

[t0yv0]

Eh, I don't love this change.

We started from a spurious warning in pulumi-aws that defines a SchemaInfo like this:

			"skip_metadata_api_check": {
				Type: "boolean",
				Default: &tfbridge.DefaultInfo{
					Value: true,
				},
			},

And actually models this filed as a String in TF land. It sounds like Pulumi->TF conversion wasn't coalescing true boolean to "true" before this change and now it does that. So far so good, and the same is extended for numbers (is there an end-user ask/ actual use case?).

The tricker bit is this:

calls MakeTerraformInput on values drawn from SchemaInfo.DefaultInfo.Value.

This has far more scope than simply coalescing bools to a desired string slot. The function is interesting, it can do things like rename object fields from Pulumi to TF spelling (although maybe it tolerates both spellings?), it can expand x to [x] in MaxItems=1 situation, and so on.

While logically it would make sense that we ask the users to write in values as Pulumi understands them into DefaultInfo.Value, it's not obvious to me that the change wouldn't break some providers that did decide to write in values as Terraform understands them to make things work, and perhaps those providers would break now, at runtime.

At the very least this needs to be called out very obviously in release notes. I'd also love to understand why perhaps this is not a concern, are we scanning existing providers for DefaultInfo.Value, or we believe that the change can't in principle break (and can demonstrate in tests).

Note that it is possible to solve the skip_metadata_api_check weakening the change to conservatively only do type coalesce operation when the type mismatches Bool != String. This opens up the possibility of taking the other improvement in a separate enhancement PR.

[iwahbe]

Eh, I don't love this change.

We started from a spurious warning in pulumi-aws that defines a SchemaInfo like this:

			"skip_metadata_api_check": {
				Type: "boolean",
				Default: &tfbridge.DefaultInfo{
					Value: true,
				},
			},

And actually models this filed as a String in TF land. It sounds like Pulumi->TF conversion wasn't coalescing true boolean to "true" before this change and now it does that. So far so good, and the same is extended for numbers (is there an end-user ask/ actual use case?).

So far so good: this change prevents the warning when the user explicitly sets skipMetadataApiCheck. The first change does not fix the error in the common case where the default value is used, because that is not processed by MakeTerraformInputs (hence the second change).

I'm not aware of a concrete use case for int->string, but it's the same problem so we should include the change for consistency. It made writing the test easier, since we can take the existing test case for MakeTerraformOutputs and assert that MakeTerraformInputs is its inverse. (modulo null uncertainty)

The tricker bit is this:

calls MakeTerraformInput on values drawn from SchemaInfo.DefaultInfo.Value.

This has far more scope than simply coalescing bools to a desired string slot. The function is interesting, it can do things like rename object fields from Pulumi to TF spelling (although maybe it tolerates both spellings?), it can expand x to [x] in MaxItems=1 situation, and so on.

While logically it would make sense that we ask the users to write in values as Pulumi understands them into DefaultInfo.Value, it's not obvious to me that the change wouldn't break some providers that did decide to write in values as Terraform understands them to make things work, and perhaps those providers would break now, at runtime.

User's are required to write in Pulumi types, since schema validation enforces that Type matches DefaultInfo.Value here.¹ This allows us to guarantee that users have not written values in TF types instead of Pulumi types; doing otherwise would error at schema validation. User's are only allowed to write simple values, not lists of objects, so MaxItemsOne and field renaming are not relevant. If we ever decided to accept collection types as DefaultInfo.Value, users would need to write pulumi values and we would then need to process them info TF values.

At the very least this needs to be called out very obviously in release notes. I'd also love to understand why perhaps this is not a concern, are we scanning existing providers for DefaultInfo.Value, or we believe that the change can't in principle break (and can demonstrate in tests).

We will call this out in the changelog. This change, like any bug fix, can break a provider that requires the old behavior. I don't expect to find this scenario in the wild. TF's frameworks expect that a stringly typed bool is represented as {"true", "false"} instead of {"1", "0"}. If we have any providers that require {"1", "0"}, they can provide a custom transformation here, but it will be nonstandard. This part of the bridge should work in the common case.

Note that it is possible to solve the skip_metadata_api_check weakening the change to conservatively only do type coalesce operation when the type mismatches Bool != String. This opens up the possibility of taking the other improvement in a separate enhancement PR.

That is a more conservative approach, and we could do that. In the absence of evidence² that this will break providers, I don't think the caution is necessary. Doing the general conversion is the more "correct" semantic, giving TF providers the types they have specified. It provides a cleaner separation between Pulumi types and TF types, which is what we want since TF providers are not aware of Pulumi types.

Footnotes

1. In the case were .Type is left empty, it is filled in with the TF value during schema generation. In that case the Pulumi type is the TF type, and .Value must be of that type. ↩

2. I'm aware that absence of evidence isn't evidence of absence, but it is suggestive of absence. ↩

[t0yv0]

Ok, I filed #1325 because it's not obvious why compound values are not supported. This was not obvious to me why that wouldn't work. Note that emitting default values into the Pulumi Package Schema seems to be an entirely optional feature to uncover more information about the provider in the documentation and generated language SDKs, as the bridge code still consults default values for runtime application. In that sense it could be possible to support compound default values just fine, which I assumed was the case.

Anyway that can wait it's time as a feature request.

For now let's merge this, the state space is a lot more constrained if compound objects are not allowed.