-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix invoke functions without args #8805
Conversation
Diff for pulumi-random with merge commit c4c545e |
Diff for pulumi-azuread with merge commit c4c545e |
Diff for pulumi-kubernetes with merge commit c4c545e |
Diff for pulumi-gcp with merge commit c4c545e |
Diff for pulumi-azure with merge commit c4c545e |
Diff for pulumi-aws with merge commit c4c545e |
Codecov Report
@@ Coverage Diff @@
## master #8805 +/- ##
=======================================
Coverage 59.39% 59.39%
=======================================
Files 639 639
Lines 98051 98066 +15
Branches 1386 1386
=======================================
+ Hits 58235 58248 +13
Misses 36533 36533
- Partials 3283 3285 +2
Continue to review full report at Codecov.
|
Diff for pulumi-azure-native with merge commit c4c545e |
Diff for pulumi-random with merge commit 8a824a2 |
Diff for pulumi-azuread with merge commit 8a824a2 |
Diff for pulumi-kubernetes with merge commit 8a824a2 |
Diff for pulumi-gcp with merge commit 8a824a2 |
Diff for pulumi-azure with merge commit 8a824a2 |
Diff for pulumi-aws with merge commit 8a824a2 |
Diff for pulumi-azuread with merge commit 3f98728 |
Diff for pulumi-random with merge commit 3f98728 |
Diff for pulumi-kubernetes with merge commit 3f98728 |
Diff for pulumi-gcp with merge commit 3f98728 |
Diff for pulumi-azure-native with merge commit 8a824a2 |
Diff for pulumi-azure with merge commit 3f98728 |
Diff for pulumi-aws with merge commit 3f98728 |
Diff for pulumi-random with merge commit a9af4ee |
Diff for pulumi-azuread with merge commit a9af4ee |
Diff for pulumi-kubernetes with merge commit a9af4ee |
Diff for pulumi-gcp with merge commit a9af4ee |
Diff for pulumi-azure-native with merge commit 3f98728 |
Diff for pulumi-azure-native with merge commit a9af4ee |
cloudWatchLogsRoleArn = invoke("aws-native::importValue", { | ||
name = "TrailLogGroupRoleTestArn" | ||
}).value | ||
kmsKeyId = invoke("aws-native::importValue", { |
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.
Program generation breaks for me unless I specify "kMSKeyId" here...but only node code generation emits a legible error message. interesting..
@@ -13,37 +13,48 @@ import ( | |||
) | |||
|
|||
func TestBindProgram(t *testing.T) { | |||
files, err := ioutil.ReadDir(testdataPath) | |||
testdata, err := ioutil.ReadDir(testdataPath) |
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.
Thanks so much for this! This test was a no-op before your fix. Quite an omission.
@@ -114,7 +118,7 @@ func (b *binder) zeroSignature() model.StaticFunctionSignature { | |||
} | |||
|
|||
func (b *binder) signatureForArgs(fn *schema.Function, args model.Expression) (model.StaticFunctionSignature, error) { | |||
if b.useOutputVersion(fn, args) { | |||
if args != nil && b.useOutputVersion(fn, args) { |
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.
The spirit of this is reasonable.
@danielrbradley do you have an issue describing the end-user problem with this? I am a little worried because I've added your example to programgen tests and they are all broken (or almost). Go, TS, Dotnet are broken. Python generates code but based on how others are broken it might have runtime errors I need to check. checking docs generation now. I will be happy to follow up with PRs that demonstrate these breakages and file issues against them I'm just wondering if this fix of yours is sufficient to unblock the use case so we can backlog fixing those things, or you need program generation to work, in which case you are still blocked? |
Example: `invoke("aws-native::getPartition")` has no additional args object passed therefore the args length is 1. Graciously fallback to disable rendering of the output form for this case.
- No sub-tests were run since the folder structure changed in 06a19b5 - Also fix capture of loop variable in closure.
This fails if 735ab2b is reverted.
- Allow example generation to run without panic while we investigate deeper issues. - Remove example which appears to have errors (leave aws-native host there for now).
ec19f20
to
4667df6
Compare
@t0yv0 the current issue is that the latest pulumi/pulumi version in aws-native causes a panic during example generation (due to accessing This is my first time looking at example generation, so may well be some additional issues with cf2pulumi that's causing the invalid pcl which I don't yet understand. I've changed direction a bit and am just emitting diagnostics if |
Diff for pulumi-random with merge commit 34b9a53 |
Diff for pulumi-azuread with merge commit 34b9a53 |
Diff for pulumi-kubernetes with merge commit 34b9a53 |
Diff for pulumi-gcp with merge commit 34b9a53 |
Diff for pulumi-azure with merge commit 34b9a53 |
Diff for pulumi-aws with merge commit 34b9a53 |
Diff for pulumi-azure-native with merge commit 34b9a53 |
@@ -80,6 +80,9 @@ func (b *binder) bindInvokeSignature(args []model.Expression) (model.StaticFunct | |||
return b.zeroSignature(), hcl.Diagnostics{unknownFunction(token, tokenRange)} | |||
} | |||
|
|||
if len(args) < 2 { | |||
return b.zeroSignature(), hcl.Diagnostics{errorf(tokenRange, "missing second arg")} |
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.
Since we're warning something actionable here perhaps?
You might expand the comment, e.g. Invoke missing a required "args" parameter, try "invoke('token', {})" instead of invoke('token')
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.
Sorry to see your example being removed? I thought that was very useful. Sorry if I was not being clear here:
-
your changes should ship I think, if
invoke("foo")
without args is intended to be supported (I think it is), good changes -
the example was uncovering further bugs in specific language programgen backends. PCL was fine but the generated code would not compile. I'd like to add that to our backlog so we can fix, I was just wondering about your use case so I don't miss the actual problem you're having we should prioritize fixing that.
TYVM.
Ah ok, shall we get this merged with the basic diagnostics so we can proceed with updating aws-native? Then I'll put my example code back onto a new branch so we can take some more time to figure out what's going on there. |
Yes no problem merging this at all. |
You have access right? Sorry if you need me to click "Squash and merge" I can do that just lmk. |
Description
Example:
invoke("aws-native::getPartition")
has no additional args object passed therefore the args length is 1.Fallback to disable rendering of the output form for this case.
Fixes #8805
Checklist