pulumi / pulumi-terraform-bridge

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

Fix unknown collections of obj #2061

Closed VenelinMartinov closed 2 months ago

VenelinMartinov commented 3 months ago

This changes the bridge to correctly return unknowns for objects and collections in sdkv2.

fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/1885 fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/2032

stacked on https://github.com/pulumi/pulumi-terraform-bridge/pull/2158

Confirmed that TF sdkv2 supports both unknown blocks and unknown collections of blocks, so we should be fine to pass these into TF providers.

The TF sdkv1 does not support unknowns for blocks and collections so we keep the old behaviour of passing empty/ collection of unknown.

```hcl provider "google" { region = "us-central1" } resource "google_storage_bucket" "bucket" { name = "example-bucket12312322312" location = "US" } resource "google_storage_bucket" "bucket1" { name = "example-bucket123123223" location = "US" dynamic "lifecycle_rule" { for_each = google_storage_bucket.bucket.effective_labels content { action { type = lifecycle_rule.value } condition { age = 1 } } } } ```

This returns "lifecycle_rule":cty.UnknownVal(cty.List(cty.Object))

Our handling of collections containing unknowns and unknown collections is significantly better:

Unknown collections: before:

+ prov:index/test:Test: (create)
  [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
  tests     : [
      [0]: {}
  ]

after:

+ prov:index/test:Test: (create)
  [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
  tests     : output<string>

Note that the array being output as an output<string> is an engine limitation.

Nested unknown collections: before:

+ prov:index/nestedTest:NestedTest: (create)
  [urn=urn:pulumi:test::test::prov:index/nestedTest:NestedTest::mainRes]
  tests     : [
      [0]: {
          nestedProps: [
              [0]: {
                  testProps : [
                      [0]: output<string>
                  ]
              }
          ]
      }
  ]

after:

+ prov:index/nestedTest:NestedTest: (create)
    [urn=urn:pulumi:test::test::prov:index/nestedTest:NestedTest::mainRes]
    tests     : [
        [0]: {
            nestedProps: [
                [0]: {
                    testProps : output<string>
                }
            ]
        }
    ]

The unknown was being put one level lower than it should be.

Quite a few other very wrong outputs in https://github.com/pulumi/pulumi-terraform-bridge/pull/2140, including diff duplications, missing diffs etc.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 33.93939% with 109 lines in your changes missing coverage. Please review.

Project coverage is 59.83%. Comparing base (4ae2ad6) to head (ddc6c06). Report is 1 commits behind head on master.

Files Patch % Lines
...g/tfbridge/internal/schemaconvert/schemaconvert.go 0.00% 86 Missing :warning:
pkg/tests/internal/pulcheck/pulcheck.go 52.38% 7 Missing and 3 partials :warning:
pf/internal/schemashim/provider.go 0.00% 2 Missing :warning:
pf/proto/unsupported.go 0.00% 2 Missing :warning:
pkg/tfshim/schema/provider.go 0.00% 2 Missing :warning:
pkg/tfshim/tfplugin5/provider.go 0.00% 2 Missing :warning:
pkg/tfshim/util/filter.go 0.00% 2 Missing :warning:
pkg/tfshim/util/util.go 0.00% 2 Missing :warning:
pkg/tfbridge/schema.go 94.11% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2061 +/- ## ========================================== - Coverage 60.74% 59.83% -0.92% ========================================== Files 353 354 +1 Lines 46146 46183 +37 ========================================== - Hits 28032 27633 -399 - Misses 16559 17002 +443 + Partials 1555 1548 -7 ```

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

VenelinMartinov commented 3 months ago

These tests were not failing last week, will take a look.

VenelinMartinov commented 3 months ago

I'm going to separate the tests here as the previous behaviour is quite different and the differences are not obvious here.

VenelinMartinov commented 2 months ago

These changes likely don't work for the terraform-plugin-sdk v1 - the tests which had to be updated use that.

v2 was released 4 years ago, wondering if we still support it.

VenelinMartinov commented 2 months ago

I've adapted this to keep the old behaviour for sdkv1 and make unknowns better in sdkv2

pulumi-bot commented 2 months ago

This PR has been shipped in release v3.88.0.