add artifact URL as soon as reconciling starts #169
Conversation
| @@ -68,10 +68,23 @@ func (r *ModelReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl | |||
| func (r *ModelReconciler) reconcileModel(ctx context.Context, model *apiv1.Model) (result, error) { | |||
| log := log.FromContext(ctx) | |||
|
|
|||
| if model.Status.Artifacts.URL != "" { | |||
| if model.Status.Ready { | |||
There was a problem hiding this comment.
Keep this change to the conditional
There was a problem hiding this comment.
you mean change to model.Status.Ready == true?
There was a problem hiding this comment.
I think what I have currently is preferred over having
if model.Status.Ready == true {
Source: https://stackoverflow.com/questions/3685002/check-if-boolean-is-true
There was a problem hiding this comment.
Was just commenting on checking ready instead of the url
There was a problem hiding this comment.
not sure why I misunderstood your comment. I read it as change to the conditional format or smth like that.
| return result{success: true}, nil | ||
| } | ||
|
|
||
| if model.Status.Artifacts.URL == "" { | ||
| model.Status.Artifacts.URL = r.Cloud.ObjectArtifactURL(model).String() |
| Reason: apiv1.ReasonJobComplete, | ||
| ObservedGeneration: model.Generation, | ||
| }) | ||
| if err := r.Status().Update(ctx, model); err != nil { |
There was a problem hiding this comment.
Remove this status update... This will be too fleeting, not worth the API request.
There was a problem hiding this comment.
This update request will be sent very soon after this and the URL will be set then.
api/v1/conditions.go
Outdated
| ConditionLoaded = "Loaded" | ||
| ConditionModelled = "Modelled" | ||
| ConditionDeployed = "Deployed" | ||
| ConditionReconciling = "Reconciling" |
There was a problem hiding this comment.
Conditions here should be end-states not transition states. IMO, I would remove this and go with the suggested changes below which make this unnecessary.
|
I verified the new approach of not using an additional update works perfectly as well. Thanks for the great suggestion |
Fixes #144