Update #11
Update #11
Conversation
| @@ -111,21 +145,30 @@ func (r *ApplicationSetReconciler) createApplications(ctx context.Context, appli | |||
| }) | |||
|
|
|||
| if err != nil { | |||
| log.Error(err, fmt.Sprintf("failed to get Application %s resource for applicationSet %s", app.Name, applicationSetInfo.Name)) | |||
| log.Error(err, fmt.Sprintf("failed to CreateOrUpdate Application %s resource for applicationSet %s", app.Name, applicationSetInfo.Name)) | |||
There was a problem hiding this comment.
We should add action in the log
| } | ||
|
|
||
| func (r *ApplicationSetReconciler) getApplications(ctx context.Context, applicationSetInfo argoprojiov1alpha1.ApplicationSet) ([]argov1alpha1.Application, error) { | ||
| // Delete apps that are not in m[string]bool |
There was a problem hiding this comment.
I think we should separate out this function , delete application shouldn't exists in apply function
| return ctrl.Result{}, err | ||
| } | ||
| r.createOrUpdateInCluster(ctx, applicationSetInfo, desiredApplications) | ||
| r.deleteInCluster(ctx, applicationSetInfo, desiredApplications) |
There was a problem hiding this comment.
Should we have some if conditions, like if finalizers is set , we should delete this cluster?
There was a problem hiding this comment.
Not sure if this is what you meant, we definitely will want to add a finalizer to ApplicationSets, and if deleting the appset we must cleanup all Applications before removing that finalizer. Deletion cleanup should probably be a separate PR. I can try to find some time for this one this week.
There was a problem hiding this comment.
this function deletes old applications from the cluster (deleted record from a list)
Deletion of ApplicationSets is implemented with owner reference so the Kubernetes GC will catch it and delete the applications
There was a problem hiding this comment.
going over the proposal, every application should have resources-finalizer.argocd.argoproj.io entry in metadata.finalizers - Added it, thanks!
Do we also want to support the option to delete a record from a list / directory from git, and not run the finalizers in the application (something like kubectl delete app NAME --cascade=false) ?
There was a problem hiding this comment.
Do we also want to support the option to delete a record from a list / directory from git, and not run the finalizers in the application (something like kubectl delete app NAME --cascade=false) ?
I think we don't need to implement this for now
There was a problem hiding this comment.
this function deletes old applications from the cluster (deleted record from a list)
Deletion of ApplicationSets is implemented with owner reference so the Kubernetes GC will catch it and delete the applications
My mistake, forgot about that!
| }) | ||
|
|
||
| if err != nil { | ||
| log.Error(err, fmt.Sprintf("failed to get Application %s resource for applicationSet %s", app.Name, applicationSetInfo.Name)) | ||
| log.Error(err, fmt.Sprintf("failed to CreateOrUpdate Application %s resource for applicationSet %s", app.Name, applicationSet.Name)) |
There was a problem hiding this comment.
Just a suggestion for using logrus and structured logging:
Define a logger with context within your loop:
appLog := log.WithFields(log.Fields{"app": app.Name, "appSet": applicationSet.Name})
Then use appLog within the loop.
appLog.WithError(err).Error("failed to CreateOrUpdate Application")
| return ctrl.Result{}, err | ||
| } | ||
| r.createOrUpdateInCluster(ctx, applicationSetInfo, desiredApplications) | ||
| r.deleteInCluster(ctx, applicationSetInfo, desiredApplications) |
There was a problem hiding this comment.
Not sure if this is what you meant, we definitely will want to add a finalizer to ApplicationSets, and if deleting the appset we must cleanup all Applications before removing that finalizer. Deletion cleanup should probably be a separate PR. I can try to find some time for this one this week.
|
LGTM. I'll merge it in. |
Refactor the create application method so it will also do update and delete.
Not sure about the name of the function, does doing delete in an apply function is ok?