Skip to content

Conversation

BigGold1310
Copy link
Contributor

Description of your changes

Adds the composed.To() method to allow an easy conversion from a function-sdk-go Unstructured to a concrete/strongly typed object.

Fixes #93

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Code is tested by unit-testing.

@BigGold1310 BigGold1310 force-pushed the main branch 3 times, most recently from c2e4a48 to 92b9428 Compare July 22, 2024 06:21
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

Thanks @BigGold1310!

Could you provide a little more context around how you'd expect to use this function? Is the idea that you'd convert observed composed resources to their real types?

When a function deserializes the Protobuf request the resources are a *structpb.Struct. Behind the scenes we're already using resource.AsObject to convert the Struct to a *composed.Unstructured when you call request.GetObservedComposedResources. Part of me wonders whether we should make it possible to go straight from Struct -> your desired type and skip the Unstructured in the middle, to avoid a layer of serialization. Not a blocker for this PR though given there's already the From function here that has a similar issue.

@BigGold1310
Copy link
Contributor Author

Thank you @negz for taking a look at this PR.

The idea of this function is to make it easier writing functions with strongly typed objects. So the function should help converting from the composed.Unstructured to a structured resource.

Converting the object directly from the *structpb.Struct sounds reasonable. I would suggest that we look at that implementation in a separate PR as more parts of the code need to be touched.
I'm happy to give it a try if you could provide me some guidance/pointers on where to look at/where to start.

@BigGold1310 BigGold1310 force-pushed the main branch 4 times, most recently from 2c5fa80 to 1b77621 Compare November 13, 2024 20:33
@BigGold1310
Copy link
Contributor Author

@negz Please review again, the DCO bot should be fixed now and I rebased to the latest version of the branch

BigGold1310 and others added 3 commits December 27, 2024 09:10
Signed-off-by: Cyrill Näf <[email protected]>
Signed-off-by: bakito <[email protected]>
Simplify error handling by directly returning the error

Co-authored-by: Nic Cope <[email protected]>
Signed-off-by: Cyrill Näf <[email protected]>
@BigGold1310
Copy link
Contributor Author

@negz Could we have some feedback on this PR? It's by now a very old one....

Comment on lines +48 to +64
// Get known GVKs for the runtime object type
knownGVKs, _, err := Scheme.ObjectKinds(obj)
if err != nil {
return errors.Errorf("could not retrieve GVKs for the provided object: %v", err)
}

// Check if GVK is known as we should not try to convert it if it doesn't match
gvkMatches := false
for _, knownGVK := range knownGVKs {
if knownGVK == un.GetObjectKind().GroupVersionKind() {
gvkMatches = true
}
}

if !gvkMatches {
return errors.Errorf("GVK %v is not known by the scheme for the provided object type", un.GetObjectKind().GroupVersionKind())
}
Copy link
Member

Choose a reason for hiding this comment

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

What would happen if we didn't check this here?

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.

Improve support of typed objects
3 participants