-
Notifications
You must be signed in to change notification settings - Fork 41
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
Conversation
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.
int
with string
Codecov ReportAttention: Patch coverage is
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. |
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.
Had a question around the integer size. Do we not need to adjust code in MakeTerraformOutput?
info: &SchemaInfo{Type: "string"}, | ||
|
||
tfInput: largeNumber, | ||
tfOutput: resource.NewProperty(strconv.FormatInt(largeNumber, 10)), |
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.
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?
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.
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
.
We do: |
This reverts commit e71c1a6. For more context, see https://github.com/pulumi/pulumi/issues/16143.
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.
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:
to convert on the wrong type.
Related to #1342
This is a pre-requisite for pulumi/pulumi-databricks#55.