-
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
Fix implicit conversion error #1945
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1945 +/- ##
=======================================
Coverage 60.51% 60.52%
=======================================
Files 329 329
Lines 44477 44479 +2
=======================================
+ Hits 26917 26922 +5
+ Misses 16050 16048 -2
+ Partials 1510 1509 -1 ☔ View full report in Codecov by Sentry. |
@t0yv0, no but I commented on the wrong PR: Actually it looks like the plugin-sdk does support int64: https://github.com/hashicorp/terraform-plugin-sdk/pull/662/files#diff-a6c63f72881e629c952253bc3ae1e7c32a0c89d7132291abc0c02b4f37d4b780R114 There's a special flag on the schema to enable that : UseJSONNumber. I'll dig some more if we are not breaking the string->int overrides. |
Reading #1940 a bit more, does this possibly apply to cases where TF represents something as a number and Pulumi chooses to represent it as a string? These are rather special cases, so it could be interesting to explain which we think is the "default" string parsing strategy, but if there's no one size-fit-all we can also make it so the provider author can override the string parsing strategy for that field (e.g. which ones are integral numbers vs floats for example). Quietly truncating is almost never a good idea, I recall some GCP issues with a big integer ID being truncated and not working anymore as an ID.. |
Note that we quietly truncate big ints here too:
This one is probably less important as it only affects env var defaults. |
Oh, lol This means I don't actually understand why my fix works as Likely some switch case going wrong somewhere. |
@@ -1245,6 +1245,8 @@ func TestOverridingTFSchema(t *testing.T) { | |||
|
|||
const largeNumber int64 = 1<<62 + 1 | |||
|
|||
const notSoLargeNumber int64 = 1<<50 + 1 |
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.
I think we are in the nasty position where TF requires that int
is 64 bit implicitly but doesn't use int64
.
hashicorp/terraform-provider-google#15507 for supporting evidence that 32 bit systems are not prioritized.
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.
Could we do notSoLargeNumber = 7
or do we need to exceed a certain threshold here? It would be great to have that in a comment.
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.
I wanted to use a number which is >32 bit but representable by float64 - so it must be <2^50-something
Looks like we handle int64 fine, so no real difference in this test really.
Okay, found the culprit: https://github.com/hashicorp/terraform-plugin-sdk/blob/1f499688ebd9420768f501d4ed622a51b2135ced/helper/schema/schema.go#L2269 The plugin sdk does type checking which does not include types like int64 etc. |
That's a great find!
That is fairly lazy. Should we consider patching that bit? |
I agree but should we rather add a layer on our side which does the conversion instead? I was under the impression we'd prefer not to patch the plugin-sdk where possible |
I'll open an issue to follow up here to make sure we use the plugin sdk correctly in all cases. I believe we should merge this as it fixes a p1 EDIT: opened #1946 |
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.
One sec
Have quick meeting but wanted to understand the comments on the 32-bit systems, are we sure this does not introduce either panics or else silent truncation, including the 32-bit systems:
My hesitation here is that the fix is not surgically scoped to the problem or the root cause, since editing makeTerraformInputs is fairly invasive and affects each and every TypeInt resource input, while the original problem was stated in terms or warnings in some special cases. |
Ah never mind, the fix only affects "retyped" fields. It's good then. |
Fixes #1940
The tf-plugin-sdk type checks input to int fields and only allows explicit ints and strings, not int64 and similar:
https://github.com/hashicorp/terraform-plugin-sdk/blob/1f499688ebd9420768f501d4ed622a51b2135ced/helper/schema/schema.go#L2269
This corrects the type there and adds tests for implicitly converting all primitive types.