More descriptive exception in serialize_property #16098
Conversation
|
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
|
Hey @Frassle! Can I get a review? |
Changelog[uncommitted] (2024-05-09)Bug Fixes
|
sdk/python/lib/pulumi/runtime/rpc.py
Outdated
| resource_obj=None, | ||
| property_key=None, |
There was a problem hiding this comment.
Should these really ever be None?
I'd think it should either be
A) Yes, some places call serialize_property and we're not in a resource and/or don't have key. In which case the raise ValueError on line 587 need to handle all those cases [obj&key, just obj, just key, neither].
B) No, everywhere should be able to pass obj and/or key and so they shouldn't default to None and shouldn't be typed as Optionals.
There was a problem hiding this comment.
Might be just one or the other for the above. Like maybe everywhere can pass "key" and it should just be of type string and not have a default, but resource really does need to be optional and default to None. Might be both need to be optional, might be neither.
There was a problem hiding this comment.
Yes! I tried passing both as positional arguments, just as you said we are calling the function in some places without any Resource and also we need to pass "key" and "object" in all the function call in that case. So I tried =None for defaults.
There was a problem hiding this comment.
In conclusion, do we want resource object default as None and "key" as Optional?
There was a problem hiding this comment.
also we need to pass "key" and "object" in all the function call in that case.
Yeh, that might be needed. I suspect "key" is needed in nearly (maybe all) call sites.
I'd probably lean on not making them defaults to start. Seeing how many places have sensible key/obj values to pass down, if any don't have a key/obj then make that property an Optional (but without a default), then if most of the callsites used None then clean it up by giving it a default.
|
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
|
Hi @Frassle, made changes
|
sdk/python/lib/pulumi/runtime/rpc.py
Outdated
| @@ -581,7 +659,9 @@ async def serialize_property( | |||
|
|
|||
| # Ensure that we have a value that Protobuf understands. | |||
| if not isLegalProtobufValue(value): | |||
| raise ValueError(f"unexpected input of type {type(value).__name__}") | |||
| raise ValueError( | |||
| f"unexpected input of type {type(value).__name__} for {property_key} in {type(resource_obj).__name__}" | |||
There was a problem hiding this comment.
property_key and resource_obj might be None. This error needs to handle that otherwise we'll get "for None in None" which doesn't read great.
There was a problem hiding this comment.
Done! Apologies, I missed this
Yes, some places call serialize_property and we're not in a resource and/or don't have key. In which case the raise ValueError on line 587 need to handle all those cases [obj&key, just obj, just key, neither].
|
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
Tentative changelog: ### Bug Fixes - [engine] Fix dataraces between snapshot and deployment systems. [#16148](#16148) - [cli/display] Fix mangled diffs of strings containing url-encoded chars [#16147](#16147) - [sdk/nodejs] Don't load punycode module in function serialization code [#16149](#16149) - [sdk/nodejs] Return error in RPC response instead of exiting in the language runtime [#16162](#16162) - [sdk/nodejs] Return the underlying error if we can't locate the pulumi SDK [#16160](#16160) - [sdk/python] Provide more descriptive exception [#16098](#16098) - [sdk/python] Fix typings for `from_input` and `all` to not return `Never` types. [#16139](#16139) - [sdk/python] Fix a race condition in output handling [#16155](#16155) ### Miscellaneous - [pkg] Upgrade pulumi-java to v0.11.0
Description
Fixes #15156
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change