Monitor.Invoke and Provider.Invoke take different arguments #9323
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AaronFriel
requested changes
Apr 13, 2022
| string provider = 3; // an optional reference to the provider version to use for this invoke. | ||
| string version = 4; // the version of the provider to use when servicing this request. | ||
| bool acceptResources = 5; // when true operations should return resource references as strongly typed. | ||
| string pluginDownloadURL = 6; // an optional reference to the provider url to use for this invoke. |
There was a problem hiding this comment.
Add a comment here that we've removed fields 3-6, and add a reserved line like reserved "provider", "version", "acceptResources", "pluginDownloadURL"? We may be able to just use 3-6, too, but to be safe reserving the names will keep those fields from being read by an old plugin.
sdk/proto/resource.proto
Outdated
| string version = 4; // the version of the provider to use when servicing this request. | ||
| bool acceptResources = 5; // when true operations should return resource references as strongly typed. | ||
| string pluginDownloadURL = 6; // an optional reference to the provider url to use for this invoke. | ||
| } |
There was a problem hiding this comment.
nit: add newline, prevents awkward diffs later
Frassle
added a commit
that referenced
this pull request
Apr 26, 2022
In a similar vein to #9323, we we're reusing the same protobuf message for sending to Monitor.Call and Provider.Call. The majority of fields on this message we're only set for the provider call, while some were only set for the monitor call.
3 tasks
justinvp
added a commit
that referenced
this pull request
Nov 16, 2022
We used to always keep resource references when marshaling the results of `pulumi:pulumi:getResource`, but this regressed in #9323 to only keep resource if request's `acceptResources` flag was set. Unfortunately, that flag was not being set when calling `pulumi:pulumi:getResource` in the Go, Node.js, and Python SDKs (although, it was being set for .NET). Two fixes: 1. Update the monitor to go back to always keeping resources when `pulumi:pulumi:getResource` is being invoked. This way, older SDKs that are not setting `acceptResources` will go back to the original behavior. 2. Update the SDKs to always set `acceptResources`, so that these newer versions of the SDKs will work with older engines that are checking `acceptResources`. (2) will help us with EKS 1.0. We'll be able to update EKS to use a version of `@pulumi/pulumi` with the fix to set the `acceptResources` flag.
justinvp
added a commit
that referenced
this pull request
Nov 16, 2022
We used to always keep resource references when marshaling the results of `pulumi:pulumi:getResource`, but this regressed in #9323 to only keep resource if request's `acceptResources` flag was set. Unfortunately, that flag was not being set when calling `pulumi:pulumi:getResource` in the Go, Node.js, and Python SDKs (although, it was being set for .NET). Two fixes: 1. Update the monitor to go back to always keeping resources when `pulumi:pulumi:getResource` is being invoked. This way, older SDKs that are not setting `acceptResources` will go back to the original behavior. 2. Update the SDKs to always set `acceptResources`, so that these newer versions of the SDKs will work with older engines that are checking `acceptResources`. (2) will help us with EKS 1.0. We'll be able to update EKS to use a version of `@pulumi/pulumi` with the fix to set the `acceptResources` flag.
bors bot
added a commit
that referenced
this pull request
Nov 16, 2022
11382: Keep resource refs when invoking `pulumi:pulumi:getResource` r=justinvp a=justinvp We used to always keep resource references when marshaling the results of `pulumi:pulumi:getResource`, but this regressed in #9323 to only keep resource if the invoke request's `acceptResources` flag was set. Unfortunately, that flag is not being set when calling `pulumi:pulumi:getResource` in the Go, Node.js, and Python SDKs (although, it is being set for .NET and Java). Two fixes: 1. Update the monitor to go back to always keeping resources when `pulumi:pulumi:getResource` is being invoked. This way, older SDKs that are not setting `acceptResources` will go back to the original behavior. 2. Update the SDKs to always set `acceptResources`, so that these newer versions of the SDKs will work with older engines that are checking `acceptResources`. (2) will help us with EKS 1.0. We'll be able to update EKS to use a version of ``@pulumi/pulumi`` with the fix to set the `acceptResources` flag. Note: We definitely need tests to lock-in the behavior here so it doesn't re-regress in the future. But I also would like to get the nodejs SDK fix in the next release, because we need it for EKS. So, I'd like to add the tests as a follow-up to this PR, unless someone feels strongly it needs to be included with this change. Fixes #11380 Co-authored-by: Justin Van Patten <jvp@justinvp.com>
abhinav
pushed a commit
to pulumi/pulumi-dotnet
that referenced
this pull request
Jan 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Monitor.Invoke and Provider.Invoke were both using the same protobuf message as their input, but they actually take different arguments.
provider,version,acceptResources, andpluginDownloadURLare only used when sending messages to the monitor. None of these made sense to send to a provider. We used to setacceptResourcesbased on the providers configuration, but providers never looked at this and the engine doesn't care because it can always accept resources.Our calls to
Invokein source_eval.go should have been usingreq.GetAcceptResources()but weren't. This also fixes that.