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

[sdk/go] Add support for untagged outputs. #4640

Merged
merged 3 commits into from
May 14, 2020

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented May 14, 2020

With these changes, a resource struct may tag a field with the empty
string. If such a field is present, any resource outputs that were not
unmarshalled into other fields will be unmarshalled into this field,
which must be a MapOutput.

Fixes #4629.

With these changes, a resource struct may tag a field with the empty
string. If such a field is present, any resource outputs that were not
unmarshalled into other fields will be unmarshalled into this field,
which must be a `MapOutput`.

Fixes #4629.
@pgavlin
Copy link
Member Author

pgavlin commented May 14, 2020

Another approach would be to add a MapOutput field to ResourceState that always contains all of the resource's outputs besides its ID and URN. That would allow consumers to meaningfully operate on Resource values in a uniform fashion. It would make it slightly more difficult for libraries to add additional ID/URN-like metadata fields, however.

Copy link
Contributor

@EvanBoyle EvanBoyle left a comment

Choose a reason for hiding this comment

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

The approach looks sound.

WRT pulumi:"" vs a consistently available member on the result, I guess I'd just ask about frequency of use. This seems like an edge case, but do we imagine external consumers using this? As long as we document it clearly since it does kinda seem like "magic".

@pgavlin
Copy link
Member Author

pgavlin commented May 14, 2020

I guess I'd just ask about frequency of use. This seems like an edge case, but do we imagine external consumers using this

The major benefit would be that this might enable libraries to more conveniently deal with resources in a generic fashion. At the moment, a package that wants to observe the outputs of an arbitrary resource needs to use reflection to iterate over its fields. Taking the approach I described would allow packages to observe the outputs of an arbitrary Resource without reflection.

Although I don't think that this is a common scenario, it would be nice to end up with one way to achieve this. If we take the approach implemented in this PR and add the suggested approach later, we'd end up with two similar but subtly different capabilities, and it might not be clear which to use when.

@lukehoban
Copy link
Member

It would make it slightly more difficult for libraries to add additional ID/URN-like metadata fields, however.

Curious - what's an example of a case where someone would (or we have yet encountered a need ourselves to) add a "ID/URN-like metadata field"?

@pgavlin
Copy link
Member Author

pgavlin commented May 14, 2020

Curious - what's an example of a case where someone would (or we have yet encountered a need ourselves to) add a "ID/URN-like metadata field"?

ApiVersion, Kind, and Metadata on the CRD resource definition:
pulumi/pulumi-kubernetes@5eab886#diff-4d25f3574f7af6916dacde7a37f85982R37-R48

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Confirmed that this worked for the k8s CustomResource.

@lblackstone
Copy link
Member

@pgavlin Anything left to do here, or is this good to merge?

@pgavlin
Copy link
Member Author

pgavlin commented May 14, 2020

This needs a CHANGELOG entry, but it's good to go other than that.

@lblackstone lblackstone merged commit e677c7d into master May 14, 2020
@pulumi-bot pulumi-bot deleted the pgavlin/remainingOutputs branch May 14, 2020 20:43
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.

[Go SDK] Add serialization support for untyped fields
4 participants