Allow Composition authors to reorder resource templates #2131
Conversation
|
CC @mcavoyk and @benagricola. I feel much better about this implementation compared to #2090. It's perhaps not the most intuitive approach but I think it makes sense given our desire to maintain the legacy behaviour. It's a smaller diff and it doesn't fork and duplicate the reconcile logic quite as much. This is ready for review implementation wise; I'm leaving it in draft for now because I'd like to update the documentation and snippets to use named templates. |
There was a problem hiding this comment.
Looking good in general. But I'm not sure if we need to consider the anonymous templates and put safeguards, like [1] [2]. Have you considered this approach that mutates Composition and composed resources to add index as template name? It'd not result in any behavior change while saving us some brain power when we try to understand the code and troubleshoot live resources.
@muvaf I did - that was my original intuition too per #2090 (comment). I decided against it because:
|
This commit allows Compositions to name their templates. When a Composition's templates are named Crossplane is able to account for more kinds of update to the template array. The array can be reordered, and elements can be added and deleted regardless of their index within the array. When a named template is deleted any composed resource that was created using that template is also deleted. Either all or no templates must be named. When no templates are named Crossplane behaves as it did before this commit was introduced; i.e. it allows templates to be appended to the templates array, but does not support reordering or deletion. Signed-off-by: Nic Cope <negz@rk0n.org>
I was initially reluctant to use 'name' because I worried that folks would interpret it as specifying the metadata.name of the composed resource, rather than just uniquely identifying the entry in the resources array. In the end I decided that I prefer 'name' to alternatives like 'id', 'templateName', or 'baseName'. API conventions prefer lists of 'named' subobjects over maps, and imply that the name field should be named 'name'. This is pretty common in Kubernetes APIs, so hopefully it won't be too confusing for folks. Signed-off-by: Nic Cope <negz@rk0n.org>
Signed-off-by: Nic Cope <negz@rk0n.org>
|
This looks really excellent @negz ! Seems to have landed on a pretty concise implementation 🎉 |
|
Thanks folks! |
Description of your changes
Fixes #1909
Closes #2090
Unblocks #1481
This commit allows Compositions to name their templates. When a Composition's templates are named Crossplane is able to account for more kinds of update to the template array. The array can be reordered, and elements can be added and deleted regardless of their index within the array. When a named template is deleted any composed resource that was created using that template is also deleted.
Either all or no templates must be named. When no templates are named Crossplane behaves as it did before this commit was introduced; i.e. it allows templates to be appended to the templates array, but does not support reordering or deletion.
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
I've spun up the contemporary "AWS with VPC" PostgreSQLInstance example from our docs, and validated that Compositions with anonymous resource templates work as they do today. I've then updated the Composition such that all templates have unique template names. When I do so the template names are propagated to the existing composed resources, and I can subsequently reorder and delete templates. When a delete a template, its corresponding composed resources are deleted.