search

pulumi / pulumi-terraform-bridge

A library allowing Terraform providers to be bridged into Pulumi.
Apache License 2.0
194 stars 43 forks source link

Allow large JSON numbers in provider2/upgradeState #2001

Closed iwahbe closed 4 months ago

iwahbe commented 4 months ago

Stacked on top of #1998

This PR allows provider2.upgradeState to handle 64 bit integer numbers. The substance of the change is https://github.com/pulumi/terraform-plugin-sdk/pull/39.

The only change here is a test.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 60.84%. Comparing base (a427483) to head (42500f6).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2001 +/- ## ========================================== - Coverage 60.85% 60.84% -0.01% ========================================== Files 332 332 Lines 44754 44754 ========================================== - Hits 27234 27231 -3 - Misses 15994 15998 +4 + Partials 1526 1525 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

t0yv0 commented 4 months ago

Help us review the SDKv2 change please. I understand you're removing an old modification from Pat, how can we confirm that the original reason for that modification to exist is removed now, that is we are not introducing bugs? Sounded like it was about custom timeout support? Any more information? Can summon Pat here.

iwahbe commented 4 months ago

Help us review the SDKv2 change please. I understand you're removing an old modification from Pat, how can we confirm that the original reason for that modification to exist is removed now, that is we are not introducing bugs? Sounded like it was about custom timeout support? Any more information? Can summon Pat here.

I've reached out to @pgavlin for context (it's 4 years ago though). The commit message mentions custom timeouts, and we have tests in place for them (https://github.com/pulumi/pulumi-terraform-bridge/pull/2001#discussion_r1608753292).

t0yv0 commented 4 months ago

Let's ship this. Please include clear messaging in the release notes about how to update to the new revision of the forked SDK. It's interesting because if providers don't build-break I assume most of the ecosystem will fail to take the update, without a reminder of a need to do so..

pgavlin commented 4 months ago

(sent this to @iwahbe in Slack as well, adding here for posterity)

I'm having difficulty remembering exactly what the issue was here. Best guesses are: