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

Presence of pulumi.DependsOn results in a runtime error #10951

Closed
phillipedwards opened this issue Oct 6, 2022 · 1 comment · Fixed by #11021
Closed

Presence of pulumi.DependsOn results in a runtime error #10951

phillipedwards opened this issue Oct 6, 2022 · 1 comment · Fixed by #11021
Assignees
Labels
area/component-packages aka multi-language components kind/bug Some behavior is incorrect or out of spec language/go resolution/fixed This issue was fixed
Milestone

Comments

@phillipedwards
Copy link
Member

phillipedwards commented Oct 6, 2022

What happened?

Presence of pulumi.DependsOn results in an error: resources.MyComponent.MyEnumProp is typed as *string but must be a type that implements pulumi.Input or pulumi.Output for input with dependencies

Steps to reproduce

Using a go generated MLC, create a simple example that creates a resource and uses pulumi.DependsOn targeting a dependent resource.

Expected Behavior

pulumi up succeeds

Actual Behavior

Program fails with resources.MyComponent.MyEnumProp is typed as *string but must be a type that implements pulumi.Input or pulumi.Output for input with dependencies

Additional context

Internal Slack Thread: https://pulumi.slack.com/archives/C01028X5X3K/p1665508743426729

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@phillipedwards phillipedwards added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Oct 6, 2022
@Frassle Frassle added language/go area/component-packages aka multi-language components and removed needs-triage Needs attention from the triage team labels Oct 7, 2022
@justinvp
Copy link
Member

When the Go SDK marshals properties, it builds-up a propertyDependencies map of property names to dependencies associated with the property. If a property doesn't have any dependencies, it's not added to the map:

pulumi/sdk/go/pulumi/rpc.go

Lines 146 to 148 in f713611

if len(allDeps) > 0 {
pdeps[pname] = allDeps.values()
}

If none of the properties have any dependencies, then the propertyDependencies will be empty.

This is different from the Node.js and Python SDKs (I need to check the behavior of the other SDKs), where the propertyDependencies map will always be filled with property names, even if a property has an empty set of dependencies.

When the engine receives a RegisterResource call, it has the following code:

if len(req.GetPropertyDependencies()) == 0 {
// If this request did not specify property dependencies, treat each property as depending on every resource
// in the request's dependency list.
for pk := range props {
propertyDependencies[pk] = dependencies
}

If the propertyDependencies is empty, it treats each property as depending on every resource in the request's dependency list (i.e. the resources specified in pulumi.DependsOn ResourceOption). This was intended for very older clients that didn't have support for propertyDependencies.

So in this particular case, there are no dependencies associated with any of the properties, so the Go SDK is sending an empty propertyDependencies, and when the engine sees that there are no property dependencies, it creates a propertyDependencies map and fills it in with dependencies from pulumi.DependsOn.

There are a few changes we should consider making here to address this:

  1. Change the Go SDK to always send a filled-in propertyDependencies, even if the properties have an empty set of dependencies.
  2. Fix the engine (CLI) to not back-fill propertyDependencies when the RegisterResource request is for a remote component -- because any SDK that supports that, already supports property dependencies, so no need to backfill.
  3. I need to dust-off and get back to this old PR to deprecate propertyDependencies when sending output values to RegisterResource: Deprecate dependency maps when using output values #8410

@justinvp justinvp self-assigned this Oct 13, 2022
@justinvp justinvp added this to the 0.79 milestone Oct 13, 2022
@bors bors bot closed this as completed in 761d1c7 Oct 14, 2022
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/component-packages aka multi-language components kind/bug Some behavior is incorrect or out of spec language/go resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants