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.

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

Introduce fn::method function for resource methods #431

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

aq17
Copy link
Contributor

@aq17 aq17 commented Dec 21, 2022

Fixes #354
Allows resource methods to be called via fn::method, using the following syntax:

variables:
  kubeconfig:
    fn::method:
      resource: ${someClusterResource}
      function: getKubeconfig
      arguments:
        ...

Both the full function type token or just the function name may be used

@aq17 aq17 changed the title Expose call attribute for resource methods Introduce fn::call function for resource methods Dec 21, 2022
@iwahbe
Copy link
Member

iwahbe commented Dec 22, 2022

It would be great if you could write up which syntax you have implemented, as there were several suggestions on the original issue. A test would likewise make the desired behavior clearer.

One thing comes to mind immediately. We have CallOpts of type InvokeOptionsDecl. I think this means we are allowing users to invoke a method on a resource of a different version/provider. I'm not sure this makes sense. @justinvp would know more if this should be allowed.

@justinvp
Copy link
Member

One thing comes to mind immediately. We have CallOpts of type InvokeOptionsDecl. I think this means we are allowing users to invoke a method on a resource of a different version/provider. I'm not sure this makes sense. @justinvp would know more if this should be allowed.

Yeah, I wouldn't expect we'd allow users to pass these. It should be passed along from the resource itself.

@aq17 aq17 force-pushed the aqiu/354 branch 3 times, most recently from 9c117e5 to 6696cef Compare January 3, 2023 18:58
@aq17 aq17 changed the title Introduce fn::call function for resource methods Introduce fn::method function for resource methods Jan 17, 2023
@iwahbe iwahbe changed the title Introduce fn::method function for resource methods Introduce fn::call function for resource methods Jan 18, 2023
@aq17 aq17 force-pushed the aqiu/354 branch 3 times, most recently from 3eda986 to c6ea6c0 Compare January 18, 2023 01:16
@aq17
Copy link
Contributor Author

aq17 commented Jan 18, 2023

@AaronFriel update: pulumi up should be working with the syntax we discussed with @justinvp
Todo:

  • testing: can we extend run_invoke_test.go , or do we need a separate framework?
  • codegen/ PCL representation

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource methods will need to be type checked, along with their arguments.

pkg/pulumiyaml/run.go Outdated Show resolved Hide resolved
@AaronFriel
Copy link
Member

AaronFriel commented Jan 18, 2023

@aq17 unfortunately that will require extending the MockResourceMonitor:

https://github.com/pulumi/pulumi/blob/9d7ccc628b2fed47ac0da67b1526bb2fed265acc/sdk/go/pulumi/mocks.go#L113-L158

It looks like due to a fluke or naming mistake, that interface uses a method named Call and implementations typically use a field named CallF to mock the Invoke RPC. That'll make adding support for mocking the Call RPC a little unusual.

@justinvp what do you think?

@justinvp
Copy link
Member

@justinvp what do you think?

This is pulumi/pulumi#8328

(Yes, it is unfortunate that the mock name for invokes is Call rather than Invoke.) If we were to add support for mocking the gRPC Call, I think the simplest thing to do would be to make the gRPC Call call the mock Call. It might seem a little strange, but having the Invoke and Call gRPCs both call the mock Call method is similar to how the RegisterResource and ReadResource gRPCs both call the mock NewResource method. (And if we needed to, at some point, we could add more fields to MockCallArgs to indicate whether it's an Invoke or Call gRPC, etc.)

The alternative is doing more design work to have a separate method in the mock just for Call. Not sure what we'd name it, though.

@aq17 aq17 force-pushed the aqiu/354 branch 3 times, most recently from 7f77fb3 to 549032d Compare January 20, 2023 23:23
@aq17 aq17 changed the title Introduce fn::call function for resource methods Introduce fn::method function for resource methods Jan 20, 2023
@t0yv0
Copy link
Member

t0yv0 commented Jul 24, 2023

👋 any updates on this, bumping into this in the context of https://github.com/t0yv0/pulumi-12709 which special cases for single-value Resource returning methods, and YAML is part of the requirement. CC @lblackstone

It sounds like there's no current workaround to call the methods in YAML, it just is not possible. Considering options here, any potential workarounds or perhaps would it be feasible for me to pick up this PR and bring it to completion. I haven't contributed to this codebase yet but can get ramped up if needed. That's an option..

I can quickly look at the possibility if YAML can pass an Output<Resource> that is a component resource output, not method call, to another resource's provider: option. This would be asymmetric to other languages but could be acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support calling resource methods
5 participants