-
Notifications
You must be signed in to change notification settings - Fork 7
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
Can't get ipAddress
field from azurenative.containerinstance.ContainerGroup
#432
Comments
This is an upstream problem (in azure-native) that bubbles up to us. Do you need the value of the ip address? If so, the only way to deal with this is to fix upstream. Our serde layer is lazy (on purpose) so if you don't use a property that fails on deserialization it won't crash your program. |
Thank you @polkx for the report. Looks like the underlying cause is two-fold:
The reason why A separate discussion is whether or not, adding an "escape hatch", like a `recover function proposed by @lbialy would be helpful to the user. As pointed out by Łukasz, due to the fact that te user has limited possibilities how to remediate such a problem, the usefulness of such escape hatch would be limited. For the reference, the schema from upstream defines the
besom azure native code:
pulumi TS azure native:
|
ipAddress
field from azurenative.containerinstance.ContainerGroup
This is what we get from the engine:
As visible above, the The behavior I would expect is:
The question is, how can we do it, and how feasible is this. |
Wait, this is an output type of struct, right? The property of a resource is an output of a case class that belongs to outputs package in codegened code? I thought that you meant that it's a property on a resource! If it's a field on a case class that's an output struct we can't do anything about it without breaking the type system. Let me elaborate - let's say we have: case class SomeProps(a: String, b: Int, c: Double)
case class SomeResource(name: Output[String], props: Output[SomeProps]) Now in serde layer we have a resource decoder that will look up decoder instances for all fields in {
"a": "some string",
"b": 23
} Notice the missing value for The only other thing we could do here is to replace all the case classes of the output-kind of structs with wrappers extending Other than that (and that's not really feasible given the Intellij problem) there's not much we can do beside complaining to the upstream to get their stuff fixed. We've had a discussion regarding hotfixes on our side that would patch schema during codegen before and you were against this. Such a hotfix to schema would allow us to mark this field as optional, codegenerate the case class with Option field and then our decoders would work without any issues. |
You are right, since output classes does not have The easiest solution in my opinion would be to have output classes with output fields, then we'd have the granularity we need in regard to decoder errors. Since we already return Resources as outputs and we have a lot of lifts and other helpers to deal with outputs, this should not be that big of an issues, or am I missing something obvious, WDYT @lbialy? Regarding other possibilities:
I'd have to dig deeper into this proposal to fully understand all the implications, but I trust your judgment and from what you're saying I'm having mixed feelings about this one, I'm not against thou, just worried about consequences to the DX.
Yes, this is always an options, unfortunately, I'd be surprised if this type of issue would get high priority, since all other SDK choose to deal with this by hiding the problem.
Yes, my main concern with schema patching is the scalability of maintenance, this would be fine with big enough community thou, but that's a bit of a chicken and the egg situation
This would effectively turn every field into optional field, because if we cannot rely on the provider to conform to its schema then we have to assume that everything is optional, and this does not sound user friendly at all to me |
There's one more consideration, the secretness. @polkx has made me aware that, when one of the fields in output class is secret, then the |
Until we re-architecture the output classes, I'd like to put under consideration the addition of |
When I try to get ipAddress (
ipAddress = containerGroup.ipAddress.ip
) from azurenative.containerinstance.ContainerGroup the Error shows up about deserializing an option.The error disappears when I comment on the line
ipAddress = containerGroup.ipAddress.ip
.Dependencies:
Main class:
Stack trace:
The text was updated successfully, but these errors were encountered: