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

Support overriding int with string #1896

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Apr 22, 2024

  • Refactor TestOverridingTFSchema for clarity.
    This commit is a pure refactor of the tests, and does not add or remove any tests.

  • Support override int with string
    This conversion was previously handled by by terraform as a best-effort approach. We
    should handle it in-house for 2 reasons:

    1. To provider errors at the pulumi level.
    2. To ensure that providers receive the same inputs as from TF, instead of relying on TF
      to convert on the wrong type.

Related to #1342

This is a pre-requisite for pulumi/pulumi-databricks#55.

This commit is a pure refactor of the tests, and does not add or remove any tests.
This conversion was previously handled by by terraform as a best-effort approach. We
should handle it in-house for 2 reasons:
1. To provider errors at the pulumi level.
2. To ensure that providers receive the same inputs as from TF, instead of relying on TF
to convert on the wrong type.
@iwahbe iwahbe requested a review from t0yv0 April 22, 2024 23:09
@iwahbe iwahbe changed the title iwahbe/test large int as string roundtrip Support overriding int with string Apr 22, 2024
@iwahbe iwahbe requested review from guineveresaenger and a team April 22, 2024 23:09
@iwahbe iwahbe self-assigned this Apr 22, 2024
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 60.19%. Comparing base (64c7a9b) to head (99a0e9f).

Files Patch % Lines
pkg/tfbridge/schema.go 92.59% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1896      +/-   ##
==========================================
+ Coverage   60.13%   60.19%   +0.06%     
==========================================
  Files         327      327              
  Lines       44093    44055      -38     
==========================================
+ Hits        26516    26521       +5     
+ Misses      16088    16043      -45     
- Partials     1489     1491       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a question around the integer size. Do we not need to adjust code in MakeTerraformOutput?

pkg/tfbridge/schema_test.go Outdated Show resolved Hide resolved
info: &SchemaInfo{Type: "string"},

tfInput: largeNumber,
tfOutput: resource.NewProperty(strconv.FormatInt(largeNumber, 10)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when Terraform gives us a large number, we transform it into a string? But if the number is small, we transform to an int64, but one of a certain size?

I'm confused because both largeNumber as well as other tf.Output values all look like int64. I presume this is a property of shim.TypeInt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when Terraform gives us a large number, we transform it into a string? But if the number is small, we transform to an int64, but one of a certain size?

We never do anything based on the size of the number given. If the TF type is an Int (int64 in Go) and the Pulumi type is "string" we ensure that the engine sees a string and that the upstream provider sees an int64. The test here is to make sure we do the conversion without going through a datatype that cannot express all of int64 (like float64).

I'm confused because both largeNumber as well as other tf.Output values all look like int64. I presume this is a property of shim.TypeInt?

strconv.FormatInt is of type func(n int64, base int) string, so tfOutput holds a string resource.PropertyValue.

pkg/tfbridge/schema_test.go Outdated Show resolved Hide resolved
pkg/tfbridge/schema.go Outdated Show resolved Hide resolved
@iwahbe
Copy link
Member Author

iwahbe commented Apr 23, 2024

Had a question around the integer size. Do we not need to adjust code in MakeTerraformOutput?

We do:

https://github.com/pulumi/pulumi-terraform-bridge/pull/1896/files#diff-5f10eecfa13490de36f3e3e71c19a2233316ebe835e2971f9a3e7d83e3b20250R1133-R1138

@iwahbe iwahbe enabled auto-merge (squash) April 23, 2024 01:02
@iwahbe iwahbe merged commit e71c1a6 into master Apr 23, 2024
11 checks passed
@iwahbe iwahbe deleted the iwahbe/test-large-int-as-string-roundtrip branch April 23, 2024 01:03
guineveresaenger added a commit that referenced this pull request May 8, 2024
guineveresaenger added a commit that referenced this pull request May 8, 2024
This reverts commit e71c1a6.

Branch off of v3.81.0 to revert #1896 and pin to providers affected by
guineveresaenger added a commit that referenced this pull request May 8, 2024
guineveresaenger added a commit to pulumi/pulumi-gcp that referenced this pull request May 8, 2024
This is a quick fix to address customer issues outlined in
pulumi/pulumi-terraform-bridge#1940.
The [bridge version
used](https://github.com/pulumi/pulumi-terraform-bridge/tree/revert-1896-for-gcp)
is based off of the last pinned bridge commit, with
pulumi/pulumi-terraform-bridge#1896
 reverted.

While we develop a more complete fix for the above issue, this will
unblock customers and enable them to upgrade.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants