fix resource refs #137
fix resource refs #137
Conversation
Signed-off-by: Bulat Shakirzyanov <83289+avalanche123@users.noreply.github.com>
Signed-off-by: Bulat Shakirzyanov <83289+avalanche123@users.noreply.github.com>
Signed-off-by: Bulat Shakirzyanov <83289+avalanche123@users.noreply.github.com>
avalanche123
force-pushed
the
fix-resource-refs
branch
from
6ad842d
to
5a9bf4c
Compare
| """ | ||
| The `ObjectReference`s for the resources composed by this composite resources. | ||
| """ | ||
| resourceRefs: [ObjectReference!]! |
There was a problem hiding this comment.
@thephred @miloszsobczak does the console need to use this new field?
There was a problem hiding this comment.
I added this for parity with other ref fields, but console could always use it using fieldPath or json fields
| if !apierrors.IsNotFound(err) { | ||
| graphql.AddError(ctx, errors.Wrap(err, errGetComposed)) | ||
| } |
There was a problem hiding this comment.
nit: add comment to xref the change in Crossplane's behaviour for context
| @@ -215,7 +215,9 @@ func (r *compositeResourceSpec) Resources(ctx context.Context, obj *model.Compos | |||
| xrc.SetKind(ref.Kind) | |||
| nn := types.NamespacedName{Namespace: ref.Namespace, Name: ref.Name} | |||
| if err := c.Get(ctx, nn, xrc); err != nil { | |||
| graphql.AddError(ctx, errors.Wrap(err, errGetComposed)) | |||
| if !apierrors.IsNotFound(err) { | |||
There was a problem hiding this comment.
An alternative would be to not consider not found a GraphQL error. We could either return a union that is the found object or not found maybe including the object ref or we could just not include not found refs as you have it here and rely on the consumer to compare with the objectrefs field to see what wasn't fetched.
The reason I'm saying all this is because for the console to consume this we'll have to turn on the feature that shows data even when there is an error and parse the error to see if it is this one somehow to keep from showing the user an error.
There was a problem hiding this comment.
for context, other refs are handled exactly this way:
|
Don't think crossplane/crossplane#4858 changed anything here. It was server side before, but with dry-run. So it was a two-phase commit before as well, with the short chance to see dangling references. There are more such places in Crossplane with that pattern. |
Description of your changes
crossplane/crossplane#4858 changed crossplane behavior to generate resource
names purely client-side, which can lead to a resource ref not resolving for
a time. This PR handles not found error and skips pending resource refs from
responses.
I have:
make reviewableto ensure this PR is ready for review.Addedbackport release-x.ylabels to auto-backport this PR, as appropriate.How has this code been tested
Going to test in a live cluster.