Refactor controllers (#1017)

* Refactor controllers

* Generated by GitHub Actions (go / generate)

https://github.com/int128/argocd-commenter/actions/runs/7382542913

* Fix

* Fix event reason

* Fix

---------

Co-authored-by: update-generated-files-action <41898282+github-actions[bot]@users.noreply.github.com>

cmd/main.go

@@ -114,7 +114,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	// comment controller
 	if err = (&controller.ApplicationPhaseCommentReconciler{
 		Client:       mgr.GetClient(),
 		Scheme:       mgr.GetScheme(),
@@ -123,6 +122,7 @@ func main() {
 		setupLog.Error(err, "unable to create controller", "controller", "ApplicationPhaseComment")
 		os.Exit(1)
 	}
+
 	if err = (&controller.ApplicationHealthCommentReconciler{
 		Client:       mgr.GetClient(),
 		Scheme:       mgr.GetScheme(),
@@ -132,7 +132,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	// deployment controller
 	if err = (&controller.ApplicationPhaseDeploymentReconciler{
 		Client:       mgr.GetClient(),
 		Scheme:       mgr.GetScheme(),
@@ -141,6 +140,7 @@ func main() {
 		setupLog.Error(err, "unable to create controller", "controller", "ApplicationPhaseDeployment")
 		os.Exit(1)
 	}
+
 	if err = (&controller.ApplicationHealthDeploymentReconciler{
 		Client:       mgr.GetClient(),
 		Scheme:       mgr.GetScheme(),
@@ -149,6 +149,7 @@ func main() {
 		setupLog.Error(err, "unable to create controller", "controller", "ApplicationHealthDeployment")
 		os.Exit(1)
 	}
+
 	if err = (&controller.ApplicationDeletionDeploymentReconciler{
 		Client:       mgr.GetClient(),
 		Scheme:       mgr.GetScheme(),

config/rbac/role.yaml

@@ -37,7 +37,6 @@ rules:
   verbs:
   - get
   - list
-  - patch
   - watch
 - apiGroups:
   - ""

internal/argocd/application.go

@@ -44,7 +44,8 @@ func GetDeploymentURL(a argocdv1alpha1.Application) string {
 	return a.Annotations["argocd-commenter.int128.github.io/deployment-url"]
 }
 
-func GetOperationPhase(a argocdv1alpha1.Application) synccommon.OperationPhase {
+// GetSyncOperationPhase returns OperationState.Phase or empty string.
+func GetSyncOperationPhase(a argocdv1alpha1.Application) synccommon.OperationPhase {
 	if a.Status.OperationState == nil {
 		return ""
 	}

internal/controller/applicationdeletiondeployment_controller.go

@@ -32,15 +32,16 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-// ApplicationDeletionDeploymentReconciler reconciles an Application object on deletion
+// ApplicationDeletionDeploymentReconciler reconciles an Application object.
+// It creates a deployment status when the Application is deleting.
 type ApplicationDeletionDeploymentReconciler struct {
 	client.Client
 	Scheme       *runtime.Scheme
 	Recorder     record.EventRecorder
 	Notification notification.Client
 }
 
-//+kubebuilder:rbac:groups=argoproj.io,resources=applications,verbs=get;watch;list;patch
+//+kubebuilder:rbac:groups=argoproj.io,resources=applications,verbs=get;watch;list
 //+kubebuilder:rbac:groups=core,resources=configmaps,verbs=get;watch;list
 //+kubebuilder:rbac:groups=core,resources=events,verbs=create;patch
 
@@ -58,23 +59,18 @@ func (r *ApplicationDeletionDeploymentReconciler) Reconcile(ctx context.Context,
 	if !isApplicationDeleting(app) {
 		return ctrl.Result{}, nil
 	}
-	logger = logger.WithValues(
-		"health", app.Status.Health.Status,
-		"deletionTimestamp", app.DeletionTimestamp,
-	)
-	ctx = log.IntoContext(ctx, logger)
 
 	argocdURL, err := argocd.GetExternalURL(ctx, r.Client, req.Namespace)
 	if err != nil {
 		logger.Info("unable to determine Argo CD URL", "error", err)
 	}
 
 	if err := r.Notification.CreateDeploymentStatusOnDeletion(ctx, app, argocdURL); err != nil {
-		logger.Error(err, "unable to create a deployment status")
-		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "CreateDeploymentError",
-			"unable to create a deployment status: %s", err)
+		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "CreateDeploymentStatusError",
+			"unable to create a deployment status on deletion: %s", err)
 	} else {
-		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedDeployment", "created a deployment status")
+		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedDeploymentStatus",
+			"created a deployment status on deletion")
 	}
 	return ctrl.Result{}, nil
 }

internal/controller/applicationhealth_controller.go

@@ -26,7 +26,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-// ApplicationHealthReconciler reconciles an Application object
+// ApplicationHealthReconciler reconciles an ApplicationHealth object
 type ApplicationHealthReconciler struct {
 	client.Client
 	Scheme *runtime.Scheme

internal/controller/applicationhealthcomment_controller.go

@@ -36,19 +36,20 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-// ApplicationHealthCommentReconciler reconciles a change of Application object
+// ApplicationHealthCommentReconciler reconciles a change of Application object.
+// It creates a comment when the health status is changed.
 type ApplicationHealthCommentReconciler struct {
 	client.Client
 	Scheme       *runtime.Scheme
 	Recorder     record.EventRecorder
 	Notification notification.Client
 }
 
-//+kubebuilder:rbac:groups=argoproj.io,resources=applications,verbs=get;watch;list;patch
+//+kubebuilder:rbac:groups=argoproj.io,resources=applications,verbs=get;watch;list
 //+kubebuilder:rbac:groups=core,resources=configmaps,verbs=get;watch;list
+//+kubebuilder:rbac:groups=core,resources=events,verbs=create;patch
 //+kubebuilder:rbac:groups=argocdcommenter.int128.github.io,resources=applicationhealths,verbs=get;list;watch;create;update;patch
 //+kubebuilder:rbac:groups=argocdcommenter.int128.github.io,resources=applicationhealths/status,verbs=get;update;patch
-//+kubebuilder:rbac:groups=core,resources=events,verbs=create;patch
 
 func (r *ApplicationHealthCommentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	logger := log.FromContext(ctx)
@@ -98,11 +99,11 @@ func (r *ApplicationHealthCommentReconciler) Reconcile(ctx context.Context, req
 	}
 
 	if err := r.Notification.CreateCommentsOnHealthChanged(ctx, app, argocdURL); err != nil {
-		logger.Error(err, "unable to create comments")
 		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "CreateCommentError",
-			"unable to create a comment by %s: %s", app.Status.Health.Status, err)
+			"unable to create a comment on health status %s: %s", app.Status.Health.Status, err)
 	} else {
-		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedComment", "created a comment by %s", app.Status.Health.Status)
+		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedComment",
+			"created a comment on health status %s", app.Status.Health.Status)
 	}
 
 	if app.Status.Health.Status != health.HealthStatusHealthy {
@@ -111,11 +112,10 @@ func (r *ApplicationHealthCommentReconciler) Reconcile(ctx context.Context, req
 	patch := client.MergeFrom(appHealth.DeepCopy())
 	appHealth.Status.LastHealthyRevision = currentRevision
 	if err := r.Client.Status().Patch(ctx, &appHealth, patch); err != nil {
-		logger.Error(err, "unable to patch lastHealthyRevision of ApplicationHealth")
+		logger.Error(err, "unable to patch lastHealthyRevision")
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
-	logger.Info("patched lastHealthyRevision of ApplicationHealth")
-	r.Recorder.Eventf(&appHealth, corev1.EventTypeNormal, "UpdateLastHealthyRevision",
+	r.Recorder.Eventf(&appHealth, corev1.EventTypeNormal, "UpdatedLastHealthyRevision",
 		"patched lastHealthyRevision to %s", currentRevision)
 	return ctrl.Result{}, nil
 }

internal/controller/applicationhealthdeployment_controller.go

@@ -45,19 +45,20 @@ var (
 	requeueTimeoutWhenDeploymentNotFound = 10 * time.Minute
 )
 
-// ApplicationHealthDeploymentReconciler reconciles an Application object
+// ApplicationHealthDeploymentReconciler reconciles an Application object.
+// It creates a deployment status when the health status is changed.
 type ApplicationHealthDeploymentReconciler struct {
 	client.Client
 	Scheme       *runtime.Scheme
 	Recorder     record.EventRecorder
 	Notification notification.Client
 }
 
-//+kubebuilder:rbac:groups=argoproj.io,resources=applications,verbs=get;watch;list;patch
+//+kubebuilder:rbac:groups=argoproj.io,resources=applications,verbs=get;watch;list
 //+kubebuilder:rbac:groups=core,resources=configmaps,verbs=get;watch;list
+//+kubebuilder:rbac:groups=core,resources=events,verbs=create;patch
 //+kubebuilder:rbac:groups=argocdcommenter.int128.github.io,resources=applicationhealths,verbs=get;list;watch;create;update;patch
 //+kubebuilder:rbac:groups=argocdcommenter.int128.github.io,resources=applicationhealths/status,verbs=get;update;patch
-//+kubebuilder:rbac:groups=core,resources=events,verbs=create;patch
 
 func (r *ApplicationHealthDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	logger := log.FromContext(ctx)
@@ -78,20 +79,19 @@ func (r *ApplicationHealthDeploymentReconciler) Reconcile(ctx context.Context, r
 	if notification.IsNotFoundError(err) {
 		// Retry until the application is synced with a valid GitHub Deployment.
 		// https://github.com/int128/argocd-commenter/issues/762
-		lastOperationAt := argocd.GetLastOperationAt(app)
-		if time.Now().Before(lastOperationAt.Add(requeueTimeoutWhenDeploymentNotFound)) {
-			logger.Info("retry due to deployment not found error", "after", requeueIntervalWhenDeploymentNotFound, "error", err)
+		lastOperationAt := argocd.GetLastOperationAt(app).Time
+		if time.Since(lastOperationAt) < requeueTimeoutWhenDeploymentNotFound {
 			r.Recorder.Eventf(&app, corev1.EventTypeNormal, "DeploymentNotFound",
 				"deployment %s not found, retry after %s", deploymentURL, requeueIntervalWhenDeploymentNotFound)
 			return ctrl.Result{RequeueAfter: requeueIntervalWhenDeploymentNotFound}, nil
 		}
-		logger.Info("retry timeout because last operation is too old", "lastOperationAt", lastOperationAt)
 		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "DeploymentNotFoundRetryTimeout",
-			"deployment %s not found, retry timeout", deploymentURL)
+			"deployment %s not found but retry timed out", deploymentURL)
 		return ctrl.Result{}, nil
 	}
 	if deploymentIsAlreadyHealthy {
-		logger.Info("skip notification because the deployment is already healthy", "deployment", deploymentURL)
+		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "DeploymentAlreadyHealthy",
+			"skip on status %s because deployment %s is already healthy", app.Status.Health.Status, deploymentURL)
 		return ctrl.Result{}, nil
 	}
 
@@ -101,11 +101,11 @@ func (r *ApplicationHealthDeploymentReconciler) Reconcile(ctx context.Context, r
 	}
 
 	if err := r.Notification.CreateDeploymentStatusOnHealthChanged(ctx, app, argocdURL); err != nil {
-		logger.Error(err, "unable to create a deployment status")
-		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "CreateDeploymentError",
-			"unable to create a deployment status by %s: %s", app.Status.Health.Status, err)
+		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "CreateDeploymentStatusError",
+			"unable to create a deployment status on health status %s: %s", app.Status.Health.Status, err)
 	} else {
-		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedDeployment", "created a deployment status by %s", app.Status.Health.Status)
+		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedDeploymentStatus",
+			"created a deployment status on health status %s", app.Status.Health.Status)
 	}
 	return ctrl.Result{}, nil
 }

internal/controller/applicationphasecomment_controller.go

@@ -32,7 +32,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-// ApplicationPhaseCommentReconciler reconciles an Application object
+// ApplicationPhaseCommentReconciler reconciles an Application object.
+// It creates a comment when the sync operation phase is changed.
 type ApplicationPhaseCommentReconciler struct {
 	client.Client
 	Scheme       *runtime.Scheme
@@ -54,19 +55,22 @@ func (r *ApplicationPhaseCommentReconciler) Reconcile(ctx context.Context, req c
 	if !app.DeletionTimestamp.IsZero() {
 		return ctrl.Result{}, nil
 	}
-	phase := argocd.GetOperationPhase(app)
+	phase := argocd.GetSyncOperationPhase(app)
+	if phase == "" {
+		return ctrl.Result{}, nil
+	}
 
 	argocdURL, err := argocd.GetExternalURL(ctx, r.Client, req.Namespace)
 	if err != nil {
 		logger.Info("unable to determine Argo CD URL", "error", err)
 	}
 
 	if err := r.Notification.CreateCommentsOnPhaseChanged(ctx, app, argocdURL); err != nil {
-		logger.Error(err, "unable to create a comment")
 		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "CreateCommentError",
-			"unable to create a comment by %s: %s", phase, err)
+			"unable to create a comment on sync operation phase %s: %s", phase, err)
 	} else {
-		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedComment", "created a comment by %s", phase)
+		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedComment",
+			"created a comment on sync operation phase %s", phase)
 	}
 	return ctrl.Result{}, nil
 }
@@ -84,7 +88,7 @@ func (r *ApplicationPhaseCommentReconciler) SetupWithManager(mgr ctrl.Manager) e
 type applicationPhaseCommentFilter struct{}
 
 func (applicationPhaseCommentFilter) Compare(applicationOld, applicationNew argocdv1alpha1.Application) bool {
-	phaseOld, phaseNew := argocd.GetOperationPhase(applicationOld), argocd.GetOperationPhase(applicationNew)
+	phaseOld, phaseNew := argocd.GetSyncOperationPhase(applicationOld), argocd.GetSyncOperationPhase(applicationNew)
 	if phaseNew == "" {
 		return false
 	}

internal/controller/applicationphasedeployment_controller.go

@@ -33,7 +33,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-// ApplicationPhaseDeploymentReconciler reconciles a ApplicationPhaseDeployment object
+// ApplicationPhaseDeploymentReconciler reconciles an Application object.
+// It creates a deployment status when the sync operation phase is changed.
 type ApplicationPhaseDeploymentReconciler struct {
 	client.Client
 	Scheme       *runtime.Scheme
@@ -55,7 +56,7 @@ func (r *ApplicationPhaseDeploymentReconciler) Reconcile(ctx context.Context, re
 	if !app.DeletionTimestamp.IsZero() {
 		return ctrl.Result{}, nil
 	}
-	phase := argocd.GetOperationPhase(app)
+	phase := argocd.GetSyncOperationPhase(app)
 	if phase == "" {
 		return ctrl.Result{}, nil
 	}
@@ -68,20 +69,19 @@ func (r *ApplicationPhaseDeploymentReconciler) Reconcile(ctx context.Context, re
 	if notification.IsNotFoundError(err) {
 		// Retry until the application is synced with a valid GitHub Deployment.
 		// https://github.com/int128/argocd-commenter/issues/762
-		lastOperationAt := argocd.GetLastOperationAt(app)
-		if time.Now().Before(lastOperationAt.Add(requeueTimeoutWhenDeploymentNotFound)) {
-			logger.Info("retry due to deployment not found error", "after", requeueIntervalWhenDeploymentNotFound, "error", err)
+		lastOperationAt := argocd.GetLastOperationAt(app).Time
+		if time.Since(lastOperationAt) < requeueTimeoutWhenDeploymentNotFound {
 			r.Recorder.Eventf(&app, corev1.EventTypeNormal, "DeploymentNotFound",
 				"deployment %s not found, retry after %s", deploymentURL, requeueIntervalWhenDeploymentNotFound)
 			return ctrl.Result{RequeueAfter: requeueIntervalWhenDeploymentNotFound}, nil
 		}
-		logger.Info("retry timeout because last operation is too old", "lastOperationAt", lastOperationAt)
 		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "DeploymentNotFoundRetryTimeout",
-			"deployment %s not found, retry timeout", deploymentURL)
+			"deployment %s not found but retry timed out", deploymentURL)
 		return ctrl.Result{}, nil
 	}
 	if deploymentIsAlreadyHealthy {
-		logger.Info("skip notification because the deployment is already healthy", "deployment", deploymentURL)
+		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "DeploymentAlreadyHealthy",
+			"skip on sync operation phase %s because deployment %s is already healthy", phase, deploymentURL)
 		return ctrl.Result{}, nil
 	}
 
@@ -91,11 +91,11 @@ func (r *ApplicationPhaseDeploymentReconciler) Reconcile(ctx context.Context, re
 	}
 
 	if err := r.Notification.CreateDeploymentStatusOnPhaseChanged(ctx, app, argocdURL); err != nil {
-		logger.Error(err, "unable to create a deployment status")
-		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "CreateDeploymentError",
-			"unable to create a deployment status by %s: %s", app.Status.Health.Status, err)
+		r.Recorder.Eventf(&app, corev1.EventTypeWarning, "CreateDeploymentStatusError",
+			"unable to create a deployment status on sync operation phase %s: %s", phase, err)
 	} else {
-		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedDeployment", "created a deployment status by %s", app.Status.Health.Status)
+		r.Recorder.Eventf(&app, corev1.EventTypeNormal, "CreatedDeploymentStatus",
+			"created a deployment status on sync operation phase %s", phase)
 	}
 	return ctrl.Result{}, nil
 }
@@ -113,7 +113,7 @@ func (r *ApplicationPhaseDeploymentReconciler) SetupWithManager(mgr ctrl.Manager
 type applicationPhaseDeploymentFilter struct{}
 
 func (applicationPhaseDeploymentFilter) Compare(applicationOld, applicationNew argocdv1alpha1.Application) bool {
-	phaseOld, phaseNew := argocd.GetOperationPhase(applicationOld), argocd.GetOperationPhase(applicationNew)
+	phaseOld, phaseNew := argocd.GetSyncOperationPhase(applicationOld), argocd.GetSyncOperationPhase(applicationNew)
 	if phaseNew == "" {
 		return false
 	}

internal/controller/suite_test.go

@@ -119,33 +119,34 @@ var _ = BeforeSuite(func() {
 	}).SetupWithManager(k8sManager)
 	Expect(err).ToNot(HaveOccurred())
 
-	// comment controllers
 	err = (&ApplicationPhaseCommentReconciler{
 		Client:       k8sManager.GetClient(),
 		Scheme:       k8sManager.GetScheme(),
 		Notification: nc,
 	}).SetupWithManager(k8sManager)
 	Expect(err).ToNot(HaveOccurred())
+
 	err = (&ApplicationHealthCommentReconciler{
 		Client:       k8sManager.GetClient(),
 		Scheme:       k8sManager.GetScheme(),
 		Notification: nc,
 	}).SetupWithManager(k8sManager)
 	Expect(err).ToNot(HaveOccurred())
 
-	// deployment controllers
 	err = (&ApplicationPhaseDeploymentReconciler{
 		Client:       k8sManager.GetClient(),
 		Scheme:       k8sManager.GetScheme(),
 		Notification: nc,
 	}).SetupWithManager(k8sManager)
 	Expect(err).ToNot(HaveOccurred())
+
 	err = (&ApplicationHealthDeploymentReconciler{
 		Client:       k8sManager.GetClient(),
 		Scheme:       k8sManager.GetScheme(),
 		Notification: nc,
 	}).SetupWithManager(k8sManager)
 	Expect(err).ToNot(HaveOccurred())
+
 	err = (&ApplicationDeletionDeploymentReconciler{
 		Client:       k8sManager.GetClient(),
 		Scheme:       k8sManager.GetScheme(),

internal/notification/phasedeployment.go

@@ -35,7 +35,7 @@ func generateDeploymentStatusOnPhaseChanged(app argocdv1alpha1.Application, argo
 		return nil
 	}
 
-	phase := argocd.GetOperationPhase(app)
+	phase := argocd.GetSyncOperationPhase(app)
 	if phase == "" {
 		return nil
 	}