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

Add support for Call #263

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Add support for Call #263

merged 1 commit into from
Aug 30, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Aug 20, 2024

This PR adds support for the Call gRPC method at the top level Provider struct.

@iwahbe iwahbe requested a review from a team August 20, 2024 12:40
@iwahbe iwahbe self-assigned this Aug 20, 2024
@t0yv0
Copy link
Member

t0yv0 commented Aug 20, 2024

What's the test plan here?

@iwahbe
Copy link
Member Author

iwahbe commented Aug 20, 2024

What's the test plan here?

I'm trying to determine the correct behavior before adding tests. We will definitely need tests before merging.

@iwahbe iwahbe force-pushed the iwahbe/call branch 2 times, most recently from ba4db4b to ed59ad1 Compare August 27, 2024 15:30
@iwahbe
Copy link
Member Author

iwahbe commented Aug 27, 2024

Tests have been added. I think this is ready to merge.

}
})

returnDependencies[string(name)] = &rpc.CallResponse_ReturnDependencies{Urns: urns}
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed even when the provider has first-class outputs in Return, where this approximation can be un-abmiguously computed? Probably no harm in including but I thought maybe engine does this?

Copy link
Member Author

@iwahbe iwahbe Aug 30, 2024

Choose a reason for hiding this comment

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

I'm not sure, but there is no documentation saying that it isn't necessary.

It seems like it is not necessary in modern Pulumi:

https://github.com/pulumi/pulumi/blob/cbfea508e7b2a4f1dd0985cefbeda3a6e891f7f9/pkg/resource/deploy/source_eval.go#L1140-L1142

I'm hesitant to assume that it is never necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened pulumi/pulumi#17113 to pin down an answer here.

@t0yv0 t0yv0 self-requested a review August 27, 2024 16:52
@t0yv0
Copy link
Member

t0yv0 commented Aug 27, 2024

Nit: I would rename the PR to drop "at the provider level", looks like this introduces support for the Call gRPC method which is great.

@t0yv0
Copy link
Member

t0yv0 commented Aug 27, 2024

One important question is instance lifetime. When a client issues a series of Call methods against a resource with the same URN, is there a possibility for the provider author to rely on the retention of in-memory instance state across the series of calls?

const c = new Counter()
c.increment()
c.increment()
export const n = c.curentValue /// 2

To achieve the above UX would it be possible for the implementation to be equally simple?

Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

Code looking good but I had a few non-blocking questions. I also would suggest considering an E2E test involving the real Pulumi CLI and a sample program, at least one of those for the happy path.

@t0yv0
Copy link
Member

t0yv0 commented Aug 27, 2024

Corner cases to think of (perhaps handled already in this codebase).

  1. Does Call issue during preview. If so how are unknowns handled?

  2. If there are schema-level or first-class secrets, does the implementation guarantee reasonable secret wrapping.

@iwahbe iwahbe changed the title Add support for Call at the Provider level Add support for Call Aug 27, 2024
@iwahbe
Copy link
Member Author

iwahbe commented Aug 30, 2024

Corner cases to think of (perhaps handled already in this codebase).

  1. Does Call issue during preview. If so how are unknowns handled?
  2. If there are schema-level or first-class secrets, does the implementation guarantee reasonable secret wrapping.
  1. I believe that it does. Call is supposed to result in a Pulumi program running (with pulumi.Output), so it unknowns should just flow through.
  2. Not at all. This implementation just brings Call support into the base level of the library. Semantic guarentees like (if secret in the schema, then secret on the wire) are handled at a higher level (Call is not hooked up to infer).

@iwahbe
Copy link
Member Author

iwahbe commented Aug 30, 2024

One important question is instance lifetime. When a client issues a series of Call methods against a resource with the same URN, is there a possibility for the provider author to rely on the retention of in-memory instance state across the series of calls?

const c = new Counter()
c.increment()
c.increment()
export const n = c.curentValue /// 2

To achieve the above UX would it be possible for the implementation to be equally simple?

I believe that this is possible now, though I would discourage relying on it. AFAIK the protocol does not make any guarantees about number of provider instances, or the order of calls. In theory, both c.increment()s could be called in parallel.

@iwahbe iwahbe force-pushed the iwahbe/call branch 3 times, most recently from ee5debd to 74b2b3f Compare August 30, 2024 15:54
@iwahbe iwahbe enabled auto-merge (rebase) August 30, 2024 15:55
@iwahbe iwahbe force-pushed the iwahbe/call branch 3 times, most recently from c2d0841 to 6136476 Compare August 30, 2024 16:03
@iwahbe iwahbe merged commit c899206 into main Aug 30, 2024
6 checks passed
@iwahbe iwahbe deleted the iwahbe/call branch August 30, 2024 16:13
@pulumi-bot
Copy link

This PR has been shipped in release v0.22.0.

@pulumi-bot
Copy link

This PR has been shipped in release v0.23.0.

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.

4 participants