Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Catch panics when typechecking warns #1987

Closed
iwahbe opened this issue May 17, 2024 · 0 comments · Fixed by #2000
Closed

Catch panics when typechecking warns #1987

iwahbe opened this issue May 17, 2024 · 0 comments · Fixed by #2000
Assignees
@corymhall
Labels
kind/enhancement Improvements or new features

Comments

@iwahbe
Copy link
Member

iwahbe commented May 17, 2024

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

Terraform providers are not expected (and do not) handle mistyped input. They often panic on mistypes inputs. When the typechecker fails, we knowingly feed them mistyped input. As long as we continue this practice, we should recover() from upstream panics when there were type checker failures.

This would reduce the impact and improve the error message when types don't line up: helping users to find and address the underlying issue faster and improving confidence in the bridge.

Follow up on #1979.

Affected area/feature
@iwahbe iwahbe added needs-triage Needs attention from the triage team kind/enhancement Improvements or new features labels May 17, 2024
@iwahbe iwahbe mentioned this issue May 17, 2024
Closed
@corymhall corymhall removed the needs-triage Needs attention from the triage team label May 17, 2024
@corymhall corymhall self-assigned this May 17, 2024
corymhall added a commit that referenced this issue May 20, 2024
@corymhall
chore: catch panics when type checking logs warning
3efddaf 
The type checker is meant to catch user configuration errors that would
eventually cause a panic. We are currently only logging a warning so we
are not preventing the panic from happening. Even though we have the
additional warning, it is still confusing to users and some do not
correlate the warning with the panic.

This PR catches the panic when we do have type errors so that the user
will only see the warning and not the panic.

closes #1987
@corymhall corymhall mentioned this issue May 20, 2024
Merged
corymhall added a commit that referenced this issue May 23, 2024
@corymhall
chore: catch panics when type checking logs warning (#2000)
872a08e 
The type checker is meant to catch user configuration errors that would
eventually cause a panic. We are currently only logging a warning so we
are not preventing the panic from happening. Even though we have the
additional warning, it is still confusing to users and some do not
correlate the warning with the panic.

This PR catches the panic when we do have type errors so that the user
will only see the warning and not the panic.

closes #1987
@VenelinMartinov VenelinMartinov mentioned this issue Sep 17, 2024
Merged
VenelinMartinov added a commit to pulumi/pulumi-gcp that referenced this issue Sep 17, 2024
@VenelinMartinov
Add regression test for type check warnings (#2389)
99484f2 
This adds a regression test for the warnings we issue to users when a
property has the wrong type. This could result in panics down the line,
so we need to catch them early.

Testing this in the bridge is currently quite hard as we can't use YAML
for this because it is too strict in type-checking. We only have unit
tests which indirectly test this but these break under PRC.

This was introduced in
pulumi/pulumi-terraform-bridge#1987
original GCP issue:
pulumi/pulumi-terraform-bridge#1979

Bridge issue for integration tests for the feature:
pulumi/pulumi-terraform-bridge#2418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@corymhall corymhall

Labels
kind/enhancement Improvements or new features
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: catch panics when type checking logs warning pulumi/pulumi-terraform-bridge
2 participants
@iwahbe @corymhall