Marshal MR structs in a (more) server-side apply compatible way #70
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Nic Cope <nicc@rk0n.org>
This is necessary for functions to be able to return JSON (as structpb.Struct) that can be used as server side apply fully specified intent. Without it we include empty structs that are non-nil, even if they have the omitempty marker. Signed-off-by: Nic Cope <nicc@rk0n.org>
I don't love the package-scoped state here but this seems like the nicest API to offer callers. Signed-off-by: Nic Cope <nicc@rk0n.org>
phisco
reviewed
Oct 30, 2023
Comment on lines
+85
to
+94
| // Unfortunately go-json-experiment also has different omitempty semantics | ||
| // for integers, in that it will include a zero-value integer with the | ||
| // omitempty tag. That will be the case for metadata.generation, so we | ||
| // delete it. It's not something a function should ever be setting. | ||
| if m, exists := obj["metadata"]; exists { | ||
| if mo, ok := m.(map[string]any); ok { | ||
| delete(mo, "generation") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
feels like we could add a cleanup step to Crossplane then probably
There was a problem hiding this comment.
You mean do something like this on the Crossplane side just in case? Yeah potentially. Maybe let's wait and see how much of an issue it will be in real life. I thought about addressing it there to start with, but it should only be an issue for Go functions and hopefully they'll all use this SDK...
phisco
approved these changes
Oct 30, 2023
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of your changes
The
composed.Fromfunction is intended to let function authors import and use MR structs from providers, e.g.:It's currently very broken in main, as demonstrated by this test:
$ go test -v -cover ./resource/composed/ === RUN Example --- PASS: Example (0.00s) === RUN ExampleFrom --- FAIL: ExampleFrom (0.00s) got: metadata: creationTimestamp: null labels: coolness: high spec: forProvider: region: us-east-2 initProvider: {} status: atProvider: {} want: apiVersion: s3.aws.upbound.io/v1beta1 kind: Bucket metadata: labels: coolness: high spec: forProvider: region: us-east-2 FAIL coverage: 12.3% of statements FAIL github.com/crossplane/function-sdk-go/resource/composed 0.015s FAILYou can see the bucket:
nullcreation timestampinitProviderandatProviderobjectsCrossplane interprets this all as part of the server-side apply fully specified intent. Specifically it will interpret it as:
It's a known issue that you can't just serialize a runtime.Object and have it be valid SSA fully-specified intent (see for example kubernetes-sigs/controller-runtime#347 (comment)). Nonetheless we're trying to do pretty much that.
I believe our MR structs are generally pretty good at making all optional fields pointers with
omitemptytags. Unfortunately the exceptions are the top-level structs likeatProvider,forProvider, etc. ObjectMeta is also known broken. So, here we use horrible hacks to make them marshal to JSON in a way that is SSA compatible.I have:
make reviewableto ensure this PR is ready for review.How has this code been tested