-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Implement a first version of try
in PCL
#18454
Conversation
5448f32
to
af8727c
Compare
try
in PCL
af8727c
to
36dfe14
Compare
pkg/codegen/pcl/functions.go
Outdated
"try": model.NewFunction(model.StaticFunctionSignature{ | ||
Parameters: []model.Parameter{{ | ||
Name: "values", | ||
Type: model.NewListType(model.DynamicType), | ||
}}, | ||
ReturnType: model.NewOutputType(model.DynamicType), | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do better here? If all the args are the same type we know the output type. For mixed types it might be better to use a union output rather than dynamic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you use GenericFunctionSignature you can even make the arg count variadic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we don't know (at this time) all the call sites of try and we are only generating one instance (now). We can do better in the future but I think for v1 we should keep it simple and try to bring types back in special cases after that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed and it's now variadic, with a slightly better inferred type signature 👍
36dfe14
to
c7ddde2
Compare
[Terraform's `try` function](https://developer.hashicorp.com/terraform/language/functions/try) accepts a variable number of arguments (of variable types) and returns the first that does not throw an error when evaluated. This change takes a first pass at adding `try` to PCL, in an effort to support better conversion from Terraform to Pulumi programs. It makes the following contributions: * PCL gets a new `try` function, which is variadic like its Terraform equivalent. An expression of the form `try(e1, e2, ..., en)` will be typed as `(t1, t2, ..., tn) -> dynamic`, where `ti` is the type of `ei`. * Code generation for NodeJS and Python has been extended to support a naive first implementation of `try`. Both implementations rely on these languages being dynamic under the hood. A Go implementation is not currently provided and its omission is tracked by #18506. * A conformance test, `l1-builtin-try`, has been added to exercise the NodeJS and Python implementations. It is skipped in Go with a reference to the aforementioned tracking issue. When this has merged, we can look at extending `pulumi-converter-terraform` to generate applications of `try`, which should enable us to make progress converting a number of popular Terraform modules and programs. Part of #18350 Co-authored-by: Brandon Pollack <brandonpollack23@gmail.com>
c7ddde2
to
76e8ff4
Compare
Terraform's
try
function accepts a variable number of arguments (of variable types) and returns the first that does not throw an error when evaluated. This change takes a first pass at addingtry
to PCL, in an effort to support better conversion from Terraform to Pulumi programs. It makes the following contributions:PCL gets a new
try
function, which takes a single argument which is a list/tuple of expressions to be tried.Code generation for NodeJS and Python has been extended to support a naive first implementation of
try
. Both implementations rely on these languages being dynamic under the hood. A Go implementation is not currently provided and its omission is tracked by Supporttry
in Go program generation #18506.A conformance test,
l1-builtin-try
, has been added to exercise the NodeJS and Python implementations. It is skipped in Go with a reference to the aforementioned tracking issue.When this has merged, we can look at extending
pulumi-converter-terraform
to generate applications oftry
, which should enable us to make progress converting a number of popular Terraform modules and programs.Part of #18350