pulumi / pulumi-terraform-bridge

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

feat: type check config values #2093

Closed corymhall closed 3 months ago

corymhall commented 3 months ago

This PR extends the typechecker to also type check config values in CheckConfig. The motivation behind this is https://github.com/pulumi/pulumi-aws/issues/4004. In that issue the user provides a key that is not in the allowed list and gets an opaque error message. In Terraform the user gets a more descriptive error message, so our behavior here is worse.

The big difference between the previous PR that is typechecking resource inputs is in that case we were preventing panics whereas in this case the primary motivation is to give better error messages. I am still only writing a warning for now which gives a message to the user like this:

  pulumi:providers:aws (provider):
    warning: Type checking failed:
    warning: Unexpected type at field "endpoints[0]":
               an unexpected argument "abcd" was provided
    warning: Type checking is still experimental. If you believe that a warning is incorrect,
    please let us know by creating an issue at https://github.com/pulumi/pulumi-terraform-bridge/issues.
    This will become a hard error in the future.
    error: pulumi:providers:aws resource 'chall-provider' has a problem: could not validate provider configuration: Invalid or unknown key. Examine values at 'chall-provider.endpoints'.

fixes https://github.com/pulumi/pulumi-aws/issues/4004

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 85.29412% with 10 lines in your changes missing coverage. Please review.

Project coverage is 61.31%. Comparing base (4785edd) to head (80ba9e3). Report is 7 commits behind head on master.

Files Patch % Lines
pkg/tfbridge/provider.go 77.27% 9 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2093 +/- ## ========================================== + Coverage 61.22% 61.31% +0.09% ========================================== Files 340 340 Lines 45143 45270 +127 ========================================== + Hits 27637 27757 +120 - Misses 15986 15994 +8 + Partials 1520 1519 -1 ```

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

t0yv0 commented 3 months ago

This is an interesting approach that might be viable! The concern as always is regressions, would it start rejecting programs that worked fine before?

Also I am curious, how does TF present this error? For other TF validation errors we had a translation layer that picked up the TF error and converted it to a Pulumi error, is there a chance that that could work here as well as an alternative approach for this specific issue, or is TF just not emitting a well labelled error?

t0yv0 commented 3 months ago

We might find type-checking helpful for config also as this might improve error messages on not-well-typed config programs, similar reasoning as deploying it for resource configurations, beyond this one particular error in AWS, though I can't quickly find example tickets pertaining to this.

corymhall commented 3 months ago

This is an interesting approach that might be viable! The concern as always is regressions, would it start rejecting programs that worked fine before?

It shouldn't! We are starting off with a warning message so we should be ok.

Also I am curious, how does TF present this error? For other TF validation errors we had a translation layer that picked up the TF error and converted it to a Pulumi error, is there a chance that that could work here as well as an alternative approach for this specific issue, or is TF just not emitting a well labelled error?

I'm not sure if Terraform has some other process for converting their error messages to user facing error messages because I couldn't trace the error message that you get from the CLI to what we are converting in the bridge. For example, this is the error you get in Terraform.

╷
│ Error: Unsupported argument
│
│   on main.tf line 14, in provider "aws":
│   14:     workspacesweb = "http://localhost:4655"
│
│ An argument named "workspacesweb" is not expected here.
╵

beyond this one particular error in AWS, though I can't quickly find example tickets pertaining to this.

Yeah I couldn't either, but this is the only good way I could think of to actually handle this type of error message.