Callback for updating status with last operation error #103
Conversation
|
|
||
| // NewAPICallbacks returns a new APICallbacks. | ||
| func NewAPICallbacks(m ctrl.Manager, of xpresource.ManagedKind) *APICallbacks { | ||
| nt := func() resource.Terraformed { |
There was a problem hiding this comment.
I opted for this pattern so that we need to keep only the metadata.name of the resource rather than the whole object accounting for @ulucinar 's concerns about memory consumption.
muvaf
force-pushed
the
will-call-you-back
branch
2 times, most recently
from
2700e76
to
1d6e10b
Compare
…rce is updated with the results of the async operation, which allows us to remove last operation error utilities from terraform.Operation Signed-off-by: Muvaffak Onus <me@muvaf.com>
muvaf
force-pushed
the
will-call-you-back
branch
from
1d6e10b
to
94f4d95
Compare
…cret utils now Signed-off-by: Muvaffak Onus <me@muvaf.com>
muvaf
force-pushed
the
will-call-you-back
branch
from
94f4d95
to
bc01a67
Compare
Signed-off-by: Muvaffak Onus <me@muvaf.com>
| return errors.Wrap(kErr, "cannot get Terraformed resource") | ||
| } | ||
| tr.SetConditions(resource.AsyncOperationCondition(err)) | ||
| return errors.Wrap(ac.kube.Status().Update(ctx, tr), errStatusUpdate) |
There was a problem hiding this comment.
Apply and Destroy function bodies are same, could we refactor not to duplicate?
There was a problem hiding this comment.
I tried that and it didn't really look better since the logic is so small anyway. Do you think I should still de-duplicate here?
There was a problem hiding this comment.
No. If you've tried it but didn't like it, let's keep it as it is.
| @@ -29,7 +29,7 @@ func Setup(mgr ctrl.Manager, l logging.Logger, rl workqueue.RateLimiter, s terra | |||
| xpresource.ManagedKind({{ .TypePackageAlias }}{{ .CRD.Kind }}GroupVersionKind), | |||
| managed.WithExternalConnecter(tjcontroller.NewConnector(mgr.GetClient(), ws, s, config.Store.GetForResource("{{ .ResourceType }}"), | |||
| {{- if .UseAsync }} | |||
| tjcontroller.UseAsync(), | |||
| tjcontroller.WithCallbackProvider(tjcontroller.NewAPICallbacks(mgr, xpresource.ManagedKind({{ .TypePackageAlias }}{{ .CRD.Kind }}GroupVersionKind))), | |||
There was a problem hiding this comment.
Couldn't we initialize this provider in NewConnector function, do we really need to pass this as an option?
I mean, we are already passing resource config which has UseAsync property and we can just use it to set a callback provider.
There was a problem hiding this comment.
We need the GVK of the resource and the mgr so that when we call newTerraformed(), the main schema can find the strong-typed Go object from its map of resources and these are not available in NewConnector. I thought of having scheme or manager and GVK as argument but didn't really want to make its argument list longer and also wanted to keep it similar to other providers where we don't need anything other than kube client in the ExternalClient implementation.
Signed-off-by: Muvaffak Onus <me@muvaf.com>
Description of your changes
Currently, when an error occurs in async calls, we'd reconcile after a whole minute to see the error, just like native async controllers like AWS RDS Instance. However, we can use the existing callback mechanism to set the error condition and that'd get us a re-queue if there is a change, which will then allow us process the resulting state.
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Manually with RDS cluster resource and EC2 Instance.