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

Fix 7862 #7887

Merged
merged 5 commits into from
Sep 7, 2021
Merged

Fix 7862 #7887

merged 5 commits into from
Sep 7, 2021

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Sep 1, 2021

Description

Fixes #7862

  • Still need to add a test, but with this change the user's problem does not repro.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

from .. import Resource


_DEPENDENCIES_PROPERTY = '_direct_computed_dependencies'
Copy link
Member Author

@t0yv0 t0yv0 Sep 1, 2021

Choose a reason for hiding this comment

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

So this algorithm needs to store the state somewhere. Options to put the state in:

  • some property of Resource objects (currently _DEPENDENCIES_PROPERTY is exactly that)
  • global table (I'm ever-so-slightly worried about leaks and re-entrancy in some auto API cases)
  • table scoped to the Pulumi program run that is executing - this would perhaps be ideal, but where do we put such state, which object/class?

Copy link
Contributor

@EvanBoyle EvanBoyle Sep 3, 2021

Choose a reason for hiding this comment

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

@pgavlin has talked about adding a kv store in the engine as a first class feature in the pulumi RPC interface to service scenarios just like this. But I don't personally have anything against the approach you've taken to solve the customer issue right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah! Hm. This would indeed come handy one day. However as things stand with Py SDK I'd be a bit worried about relying on async KV store for core functionality.. For example I'd love to first get rid of threading and have a really foolproof async situation. in current design core functionality of Py SDK is to order the RegisterResource calls "right" before they hit the engine.

So far I can think of no downside piggybacking on the Resource prop - we're not ever pickling these objects right.. But if needed/wanted I can also explicitly declare this prop on the Resource class so it's more official this is being used.

@t0yv0 t0yv0 marked this pull request as ready for review September 1, 2021 20:20
@t0yv0 t0yv0 merged commit d7b9c01 into master Sep 7, 2021
@pulumi-bot pulumi-bot deleted the t0yv0/fix-7862 branch September 7, 2021 15:30
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.

Pulumi v3.10.1+ hangs forever when previewing
2 participants