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

makeTerraformInputs converts empty strings wrong #1916

Closed
VenelinMartinov opened this issue Apr 29, 2024 · 7 comments · Fixed by #1998
Closed

makeTerraformInputs converts empty strings wrong #1916

VenelinMartinov opened this issue Apr 29, 2024 · 7 comments · Fixed by #1998
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed

Comments

@VenelinMartinov
Copy link
Contributor

What happened?

discovered in #1913

We convert empty strings wrong.

Example

func TestInputsEmptyString(t *testing.T) {
	t.Skip("Empty strings are misrepresented")
	runCreateInputCheck(t, inputTestCase{
		Resource: &schema.Resource{
			Schema: map[string]*schema.Schema{
				"f0": {
					Type:     schema.TypeString,
					Required: true,
				},
			},
		},
		Config: tftypes.NewValue(
			tftypes.Object{
				AttributeTypes: map[string]tftypes.Type{
					"f0": tftypes.String,
				},
			},
			map[string]tftypes.Value{
				"f0": tftypes.NewValue(tftypes.String, ""),
			}),
	})
}

Output of pulumi about

.

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@VenelinMartinov VenelinMartinov added needs-triage Needs attention from the triage team kind/bug Some behavior is incorrect or out of spec and removed needs-triage Needs attention from the triage team labels Apr 29, 2024
@VenelinMartinov VenelinMartinov changed the title makeTerraformInputs converts empty lists wrong makeTerraformInputs converts empty strings wrong Apr 29, 2024
@iwahbe
Copy link
Member

iwahbe commented May 13, 2024

@VenelinMartinov How do we convert empty strings vs what they should be? Can you post a diff (what we do vs what tf does)?

@t0yv0
Copy link
Member

t0yv0 commented May 16, 2024

I'd be curious to have some more detail here as well. I think I have an intuition about why set-nested blocks should convert as empty sets instead of null, but strings?

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented May 16, 2024

This is what I got in TestInputsEmptyString:

Resource: &schema.Resource{
	Schema: map[string]*schema.Schema{
		"f0": {
			Type:     schema.TypeString,
			Required: true,
		},
	},
},
Config: tftypes.NewValue(
	tftypes.Object{
		AttributeTypes: map[string]tftypes.Type{
			"f0": tftypes.String,
		},
	},
	map[string]tftypes.Value{
		"f0": tftypes.NewValue(tftypes.String, ""),
	},
),
TF value cty.ObjectVal(map[string]cty.Value{"f0":cty.StringVal(""), "id":cty.UnknownVal(cty.String)})
PU value cty.ObjectVal(map[string]cty.Value{"f0":cty.NullVal(cty.String), "id":cty.NullVal(cty.String)})

We seem to use NullVal(cty.String) while TF returns cty.StringVal("") in RawPlan

@t0yv0
Copy link
Member

t0yv0 commented May 16, 2024

That's under Required: true? Does it hold under Optional: true?

@t0yv0
Copy link
Member

t0yv0 commented May 16, 2024

Wait hold on a second, Pulumi substitutes null for ""? That's very surprising. Might be worth double checking that we print a correct Pulumi program and it's not some kind of test harness issue.

@VenelinMartinov
Copy link
Contributor Author

backend:
    url: file://./data
name: project
resources:
    example:
        properties:
            f0: ""
        type: crossprovider:index:TestRes
runtime: yaml
resource "crossprovider_testres" "example" {
  f0 = ""
}

Same thing for Optional: true.

I've also checked that #1971 fixes this so it must be UpgradeState at fault here.

@t0yv0
Copy link
Member

t0yv0 commented May 17, 2024

I'm a little mindblown and very glad we got a fix for this.

VenelinMartinov added a commit that referenced this issue May 21, 2024
fixes #1916
fixes #1914

This rewrites the provider2 upgrade state implementation to use the
upstream GRPC method instead of the bridge implementation. This should
cut out a bunch of unnecessary code and some type conversions of the
data.

Originally done as part of
#1971 but I'm
pulling this out to merge separately.

@t0yv0 did a lot of the heavy lifting here.
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants