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
Keep resource refs when invoking pulumi:pulumi:getResource
#11382
Conversation
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.
accept_resources = not ( | ||
os.getenv("PULUMI_DISABLE_RESOURCE_REFERENCES", "").upper() | ||
in {"TRUE", "1"} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's debatable whether we even need to continue respecting PULUMI_DISABLE_RESOURCE_REFERENCES
(especially in these new cases), which was used when we were originally rolling out resource refs as a way to be able to turn off the feature if something went wrong during the rollout. We still check the envvar elsewhere, so I'm continuing to use it in these new places. Separately, we should consider removing it throughout as it's not really needed anymore (resource refs have been working fine E2E -- there hasn't been a need to disable them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go with this as is for now. Sounds like a good thing to do a cleanup pass on though.
bors merge |
Build succeeded: |
Late to this - but can/should this have test coverage to ensure we don't regress again? |
Absolutely. I noted this in the description and have a tracking issue (#11384) to do that as a follow-up in the next couple of days; but I wanted to try to get the fix in for today's release.
|
The explanation for this fix is 💯, lovely to see that.
It's still good to document some evidence that the fix fixes the problem. So what you could do, if regression tests will take a while longer, is write down steps for a reviewer to verify for themselves that it works. Just writing down what you did to try it out is often good enough for this purpose (and serves as a kind of self-certification). |
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'sacceptResources
flag was set. Unfortunately, that flag is not being set when callingpulumi:pulumi:getResource
in the Go, Node.js, and Python SDKs (although, it is being set for .NET and Java).Two fixes:
pulumi:pulumi:getResource
is being invoked. This way, older SDKs that are not settingacceptResources
will go back to the original behavior.acceptResources
, so that these newer versions of the SDKs will work with older engines that are checkingacceptResources
.(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 theacceptResources
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