pulumi / pulumi-terraform-bridge

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

Get new host client directly when attaching a muxed provider #2091

Closed guineveresaenger closed 3 months ago

guineveresaenger commented 3 months ago

Fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/2088.

t0yv0 commented 3 months ago

TBH I don't quite understand the fix and I think this still a borderline call whether this is a user-facing panic or not, I think it is since https://www.pulumi.com/docs/using-pulumi/pulumi-packages/debugging-provider-packages/ documents the feature for the user.

This issue was claimed to be fixed:

https://github.com/pulumi/pulumi-terraform-bridge/issues/1293 https://github.com/pulumi/pulumi-terraform-bridge/pull/1299

I recall referencing something in AWS claiming that the fix is coming. Since it's regressed, I'd like to request a test, and an RCA we can all understand. CC @mjeffryes for a second opinion.

t0yv0 commented 3 months ago

Ok the ticket also mentions https://github.com/pulumi/pulumi-terraform-bridge/pull/1716 as the prior attempt to fix subsequently to 1299/1293.

That included a test that was skipped until https://github.com/pulumi/pulumi/pull/15526 which is now in.

Can this test be un-skipped now, looks like I failed to follow up on that since 15526 went in? Was this test not covering the feature properly or is it in fact failing once un-skipped?

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 61.25%. Comparing base (388299d) to head (97ab429). Report is 3 commits behind head on master.

Files Patch % Lines
x/muxer/muxer.go 0.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2091 +/- ## ========================================== + Coverage 61.21% 61.25% +0.04% ========================================== Files 339 340 +1 Lines 45140 45161 +21 ========================================== + Hits 27633 27664 +31 + Misses 15987 15975 -12 - Partials 1520 1522 +2 ```

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

t0yv0 commented 3 months ago

It's better! Could we please re-enable TestAttach test and add one for muxed providers at integration level? Much appreciated.

danielrbradley commented 3 months ago

Would love a brief explanation on why and how this fixes the issue. My go-foo is lacking here.

It's a beautiful one line fix .. but I have no idea why it would fix it 🙃

Perhaps as a code comment to avoid something thinking this code should be a simple nil check and 'fixing' it!

t0yv0 commented 3 months ago

http://golang50shad.es/index.html#nil_in_nil_in_vals I think. It truly is non-intuitive.