diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 471d237a..3dc1c63c 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -19,11 +19,10 @@ package actionsgithubcom import ( "context" "fmt" - "maps" - "reflect" "time" "github.com/go-logr/logr" + "github.com/google/go-cmp/cmp" kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -78,7 +77,6 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err := r.Get(ctx, req.NamespacedName, &autoscalingListener); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - var original once[*v1alpha1.AutoscalingListener] if !autoscalingListener.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { @@ -93,15 +91,15 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } if requeue { log.Info("Waiting for resources to be deleted before removing finalizer") - return ctrl.Result{Requeue: true, RequeueAfter: time.Second}, nil + return ctrl.Result{RequeueAfter: time.Second}, nil } - removeFinalizer := controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) - if removeFinalizer { - original.Do(autoscalingListener.DeepCopy) + original := newOnce(autoscalingListener.DeepCopy) + if controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { + original.Do() controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) } - if removeFinalizer { + if original.Called() { log.Info("Removing finalizer") if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original.Get())); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to remove finalizer") @@ -114,19 +112,19 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } + original := newOnce(autoscalingListener.DeepCopy) addFinalizer := !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) if addFinalizer { - original.Do(autoscalingListener.DeepCopy) + original.Do() controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) } - if addFinalizer { + if original.Called() { if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to add finalizer") return ctrl.Result{}, err } log.Info("Successfully added finalizer") - return ctrl.Result{}, nil } // Check if the AutoscalingRunnerSet exists @@ -174,28 +172,40 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - desiredLabels := r.filterAndMergeLabels(serviceAccount.Labels, desiredServiceAccount.Labels) - labelsModified := !maps.Equal(serviceAccount.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(serviceAccount.Annotations, desiredServiceAccount.Annotations) - annotationsModified := !maps.Equal(serviceAccount.Annotations, desiredAnnotations) - var original once[*corev1.ServiceAccount] + desiredLabels, labelsModified := r.mergeLabels(serviceAccount.Labels, desiredServiceAccount.Labels) + original := newOnce(serviceAccount.DeepCopy) if labelsModified { - original.Do(serviceAccount.DeepCopy) + original.Do() serviceAccount.Labels = desiredLabels } + desiredAnnotations, annotationsModified := r.mergeAnnotations(serviceAccount.Annotations, desiredServiceAccount.Annotations) if annotationsModified { - original.Do(serviceAccount.DeepCopy) + original.Do() serviceAccount.Annotations = desiredAnnotations } - if labelsModified || annotationsModified { + secretsModified := !cmp.Equal(serviceAccount.Secrets, desiredServiceAccount.Secrets) + if secretsModified { + original.Do() + serviceAccount.Secrets = desiredServiceAccount.Secrets + } + imagePullSecretsModified := !cmp.Equal(serviceAccount.ImagePullSecrets, desiredServiceAccount.ImagePullSecrets) + if imagePullSecretsModified { + original.Do() + serviceAccount.ImagePullSecrets = desiredServiceAccount.ImagePullSecrets + } + automountServiceAccountTokenModified := !cmp.Equal(serviceAccount.AutomountServiceAccountToken, desiredServiceAccount.AutomountServiceAccountToken) + if automountServiceAccountTokenModified { + original.Do() + serviceAccount.AutomountServiceAccountToken = desiredServiceAccount.AutomountServiceAccountToken + } + + if original.Called() { log.Info("Updating listener service account") if err := r.Patch(ctx, &serviceAccount, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener service account") return ctrl.Result{}, err } - - return ctrl.Result{Requeue: true}, nil } case kerrors.IsNotFound(err): // Create a service account for the listener pod in the controller namespace @@ -218,32 +228,29 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: + original := newOnce(listenerRole.DeepCopy) desiredRole := r.newScaleSetListenerRole(&autoscalingListener) - desiredLabels := r.filterAndMergeLabels(listenerRole.Labels, desiredRole.Labels) - labelsModified := !maps.Equal(listenerRole.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerRole.Annotations, desiredRole.Annotations) - annotationsModified := !maps.Equal(listenerRole.Annotations, desiredAnnotations) - rulesModified := !reflect.DeepEqual(listenerRole.Rules, desiredRole.Rules) - var original once[*rbacv1.Role] + desiredLabels, labelsModified := r.mergeLabels(listenerRole.Labels, desiredRole.Labels) if labelsModified { - original.Do(listenerRole.DeepCopy) + original.Do() listenerRole.Labels = desiredLabels } + desiredAnnotations, annotationsModified := r.mergeAnnotations(listenerRole.Annotations, desiredRole.Annotations) if annotationsModified { - original.Do(listenerRole.DeepCopy) + original.Do() listenerRole.Annotations = desiredAnnotations } + rulesModified := !cmp.Equal(listenerRole.Rules, desiredRole.Rules) if rulesModified { - original.Do(listenerRole.DeepCopy) + original.Do() listenerRole.Rules = desiredRole.Rules } - if labelsModified || annotationsModified || rulesModified { + if original.Called() { log.Info("Updating listener role") if err := r.Patch(ctx, &listenerRole, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener role") return ctrl.Result{}, err } - return ctrl.Result{Requeue: true}, nil } case kerrors.IsNotFound(err): // Create a role for the listener pod in the AutoScalingRunnerSet namespace @@ -259,35 +266,42 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, &listenerRoleBinding) switch { case err == nil: + original := newOnce(listenerRoleBinding.DeepCopy) desiredRoleBinding := r.newScaleSetListenerRoleBinding( &autoscalingListener, &listenerRole, &serviceAccount, ) - desiredLabels := r.filterAndMergeLabels(listenerRoleBinding.Labels, desiredRoleBinding.Labels) - labelsModified := !maps.Equal(listenerRoleBinding.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerRoleBinding.Annotations, desiredRoleBinding.Annotations) - annotationsModified := !maps.Equal(listenerRoleBinding.Annotations, desiredAnnotations) - var original once[*rbacv1.RoleBinding] + desiredLabels, labelsModified := r.mergeLabels(listenerRoleBinding.Labels, desiredRoleBinding.Labels) if labelsModified { - original.Do(listenerRoleBinding.DeepCopy) + original.Do() listenerRoleBinding.Labels = desiredLabels } + + desiredAnnotations, annotationsModified := r.mergeAnnotations(listenerRoleBinding.Annotations, desiredRoleBinding.Annotations) if annotationsModified { - original.Do(listenerRoleBinding.DeepCopy) + original.Do() listenerRoleBinding.Annotations = desiredAnnotations } - if labelsModified || annotationsModified { + rulesModified := !cmp.Equal(listenerRoleBinding.RoleRef, desiredRoleBinding.RoleRef) + if rulesModified { + original.Do() + listenerRoleBinding.RoleRef = desiredRoleBinding.RoleRef + } + + subjectsModified := !cmp.Equal(listenerRoleBinding.Subjects, desiredRoleBinding.Subjects) + if subjectsModified { + original.Do() + listenerRoleBinding.Subjects = desiredRoleBinding.Subjects + } + + if original.Called() { log.Info("Updating listener role binding") if err := r.Patch(ctx, &listenerRoleBinding, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener role binding") return ctrl.Result{}, err } - - log.Info("Updated listener role binding") - return ctrl.Result{Requeue: true}, nil } - case kerrors.IsNotFound(err): // Create a role binding for the listener pod in the AutoScalingRunnerSet namespace log.Info("Creating a role binding for the service account and role") @@ -316,31 +330,47 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: + original := newOnce(proxySecret.DeepCopy) desiredListenerProxy, err := r.newAutoscalingListenerProxySecret(&autoscalingListener, proxySecret.Data) if err != nil { log.Error(err, "Failed to build desired listener proxy secret") return ctrl.Result{}, err } - desiredLabels := r.filterAndMergeLabels(proxySecret.Labels, desiredListenerProxy.Labels) - labelsModified := !maps.Equal(proxySecret.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(proxySecret.Annotations, desiredListenerProxy.Annotations) - annotationsModified := !maps.Equal(proxySecret.Annotations, desiredAnnotations) - var original once[*corev1.Secret] + desiredLabels, labelsModified := r.mergeLabels(proxySecret.Labels, desiredListenerProxy.Labels) if labelsModified { - original.Do(proxySecret.DeepCopy) + original.Do() proxySecret.Labels = desiredLabels } + desiredAnnotations, annotationsModified := r.mergeAnnotations(proxySecret.Annotations, desiredListenerProxy.Annotations) if annotationsModified { - original.Do(proxySecret.DeepCopy) + original.Do() proxySecret.Annotations = desiredAnnotations } - if labelsModified || annotationsModified { + // we set the data so we just need to check other fields are nil + if proxySecret.Immutable != nil { + original.Do() + proxySecret.Immutable = nil + } + if proxySecret.StringData != nil { + original.Do() + proxySecret.StringData = nil + } + if proxySecret.Type != desiredListenerProxy.Type { + original.Do() + proxySecret.Type = desiredListenerProxy.Type + } + dataModified := !cmp.Equal(proxySecret.Data, desiredListenerProxy.Data) + if dataModified { + original.Do() + proxySecret.Data = desiredListenerProxy.Data + } + + if original.Called() { log.Info("Updating listener proxy secret") if err := r.Patch(ctx, &proxySecret, client.MergeFrom(original.Get())); err != nil { log.Error(err, "Failed to update listener proxy secret") return ctrl.Result{}, err } - return ctrl.Result{Requeue: true}, nil } case kerrors.IsNotFound(err): // Create a mirror secret for the listener pod in the Controller namespace for listener pod to use @@ -363,10 +393,8 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. log.Error( err, "Failed to get app config for AutoscalingRunnerSet.", - "namespace", - autoscalingRunnerSet.Namespace, - "name", - autoscalingRunnerSet.GitHubConfigSecret, + "namespace", autoscalingRunnerSet.Namespace, + "name", autoscalingRunnerSet.GitHubConfigSecret, ) return nil, err } @@ -394,6 +422,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: + original := newOnce(listenerConfigSecret.DeepCopy) cfg, err := r.GetAppConfig(ctx, &autoscalingRunnerSet) if err != nil { return ctrl.Result{}, err @@ -409,21 +438,31 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err != nil { return ctrl.Result{}, fmt.Errorf("failed to build listener config secret: %w", err) } - desiredLabels := r.filterAndMergeLabels(listenerConfigSecret.Labels, desiredSecret.Labels) - labelsModified := !maps.Equal(listenerConfigSecret.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerConfigSecret.Annotations, desiredSecret.Annotations) - annotationsModified := !maps.Equal(listenerConfigSecret.Annotations, desiredAnnotations) - var original once[*corev1.Secret] + desiredLabels, labelsModified := r.mergeLabels(listenerConfigSecret.Labels, desiredSecret.Labels) if labelsModified { - original.Do(listenerConfigSecret.DeepCopy) + original.Do() listenerConfigSecret.Labels = desiredLabels } + desiredAnnotations, annotationsModified := r.mergeAnnotations(listenerConfigSecret.Annotations, desiredSecret.Annotations) if annotationsModified { - original.Do(listenerConfigSecret.DeepCopy) + original.Do() listenerConfigSecret.Annotations = desiredAnnotations } + // we set the data so we just need to check other fields are nil + if listenerConfigSecret.Immutable != nil { + original.Do() + listenerConfigSecret.Immutable = nil + } + if listenerConfigSecret.StringData != nil { + original.Do() + listenerConfigSecret.StringData = nil + } + if listenerConfigSecret.Type != desiredSecret.Type { + original.Do() + listenerConfigSecret.Type = desiredSecret.Type + } - if labelsModified || annotationsModified { + if original.Called() { log.Info("Updating listener config secret", "namespace", listenerConfigSecret.Namespace, "name", listenerConfigSecret.Name) if err := r.Patch(ctx, &listenerConfigSecret, client.MergeFrom(original.Get())); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update listener config secret: %w", err) @@ -454,7 +493,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } // Requeue to create listener pod with the config secret - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{RequeueAfter: 100 * time.Millisecond}, nil default: log.Error(err, "Unable to get listener config secret", "namespace", autoscalingListener.Namespace, "name", scaleSetListenerConfigName(&autoscalingListener)) return ctrl.Result{}, err @@ -484,17 +523,15 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - desiredLabels := r.filterAndMergeLabels(listenerPod.Labels, desiredPod.Labels) - labelsModified := !maps.Equal(listenerPod.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) - annotationsModified := !maps.Equal(listenerPod.Annotations, desiredAnnotations) - var original once[*corev1.Pod] + original := newOnce(listenerPod.DeepCopy) + desiredLabels, labelsModified := r.mergeLabels(listenerPod.Labels, desiredPod.Labels) if labelsModified { - original.Do(listenerPod.DeepCopy) + original.Do() listenerPod.Labels = desiredLabels } + desiredAnnotations, annotationsModified := r.mergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) if annotationsModified { - original.Do(listenerPod.DeepCopy) + original.Do() listenerPod.Annotations = desiredAnnotations } diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 5b98e2c5..2a496d68 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -19,7 +19,6 @@ package actionsgithubcom import ( "context" "fmt" - "maps" "strconv" "strings" "time" @@ -74,7 +73,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if err := r.Get(ctx, req.NamespacedName, &autoscalingRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - var original once[*v1alpha1.AutoscalingRunnerSet] + original := newOnce(autoscalingRunnerSet.DeepCopy) if !autoscalingRunnerSet.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { @@ -101,7 +100,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl removeFinalizer := controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) if removeFinalizer { - original.Do(autoscalingRunnerSet.DeepCopy) + original.Do() controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) } if removeFinalizer { @@ -137,7 +136,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl addFinalizer := !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) if addFinalizer { - original.Do(autoscalingRunnerSet.DeepCopy) + original.Do() controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) } if addFinalizer { @@ -205,15 +204,15 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - var original once[*v1alpha1.EphemeralRunnerSet] + original := newOnce(ephemeralRunnerSet.DeepCopy) ephemeralRunnerReplicasModified := ephemeralRunnerSet.Spec.Replicas != 0 if ephemeralRunnerReplicasModified { - original.Do(ephemeralRunnerSet.DeepCopy) + original.Do() ephemeralRunnerSet.Spec.Replicas = 0 } ephemeralRunnerPatchIDModified := ephemeralRunnerSet.Spec.PatchID != 0 if ephemeralRunnerPatchIDModified { - original.Do(ephemeralRunnerSet.DeepCopy) + original.Do() ephemeralRunnerSet.Spec.PatchID = 0 } if ephemeralRunnerReplicasModified || ephemeralRunnerPatchIDModified { @@ -302,29 +301,29 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - var original once[*v1alpha1.EphemeralRunnerSet] + original := newOnce(ephemeralRunnerSet.DeepCopy) ephemeralRunnerActionableSpecModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerSpec, desired.Spec.EphemeralRunnerSpec) if ephemeralRunnerActionableSpecModified { - original.Do(ephemeralRunnerSet.DeepCopy) + original.Do() ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec ephemeralRunnerSet.Spec.ActionableRevision = nextActionableRevision(&ephemeralRunnerSet) } ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata) if ephemeralRunnerMetadataModified { - original.Do(ephemeralRunnerSet.DeepCopy) + original.Do() ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata } - ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels) + desiredLabels, ephemeralRunnerLabelsModified := r.mergeLabels(ephemeralRunnerSet.Labels, desired.Labels) if ephemeralRunnerLabelsModified { - original.Do(ephemeralRunnerSet.DeepCopy) - ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) + original.Do() + ephemeralRunnerSet.Labels = desiredLabels } - ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations) + desiredAnnotations, ephemeralRunnerAnnotationsModified := r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) if ephemeralRunnerAnnotationsModified { - original.Do(ephemeralRunnerSet.DeepCopy) - ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) + original.Do() + ephemeralRunnerSet.Annotations = desiredAnnotations } if ephemeralRunnerActionableSpecModified || ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { @@ -373,9 +372,20 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if !cmp.Equal(listener.Spec, desired.Spec) || - !cmp.Equal(listener.Labels, desired.Labels) || - !cmp.Equal(listener.Annotations, desired.Annotations) { + original := newOnce(listener.DeepCopy) + desiredLabels, listnerLabelsModified := r.mergeLabels(listener.Labels, desired.Labels) + if listnerLabelsModified { + original.Do() + listener.Labels = desiredLabels + } + + desiredAnnotations, listenerAnnotationsModified := r.mergeAnnotations(listener.Annotations, desired.Annotations) + if listenerAnnotationsModified { + original.Do() + listener.Annotations = desiredAnnotations + } + + if !cmp.Equal(listener.Spec, desired.Spec) { log.Info("Deleting AutoscalingListener to re-create with updated spec") if err := r.Delete(ctx, &listener); err != nil { log.Error(err, "Failed to delete AutoscalingListener for re-creation") @@ -384,6 +394,16 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl log.Info("Deleted AutoscalingListener, will re-create on next reconcile") return ctrl.Result{}, nil } + + if original.Called() { + log.Info("Updating AutoscalingListener metadata to match desired labels and annotations") + if err := r.Patch(ctx, &listener, client.MergeFrom(original.Get())); err != nil { + log.Error(err, "Failed to patch AutoscalingListener metadata") + return ctrl.Result{}, err + } + log.Info("Successfully patched AutoscalingListener metadata") + return ctrl.Result{}, nil + } } log.Info("Autoscaling runner set is up to date and ready") @@ -536,11 +556,11 @@ func (r *AutoscalingRunnerSetReconciler) removeFinalizersFromDependentResources( } func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) (ctrl.Result, error) { - var original once[*v1alpha1.AutoscalingRunnerSet] + original := newOnce(autoscalingRunnerSet.DeepCopy) logger.Info("Creating a new runner scale set") actionsClient, err := r.GetActionsService(ctx, autoscalingRunnerSet) if len(autoscalingRunnerSet.Spec.RunnerScaleSetName) == 0 { - original.Do(autoscalingRunnerSet.DeepCopy) + original.Do() autoscalingRunnerSet.Spec.RunnerScaleSetName = autoscalingRunnerSet.Name } if err != nil { @@ -615,7 +635,7 @@ func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Contex actionsClient.SetSystemInfo(info) logger.Info("Created/Reused a runner scale set", "id", runnerScaleSet.ID, "runnerGroupName", runnerScaleSet.RunnerGroupName) - original.Do(autoscalingRunnerSet.DeepCopy) + original.Do() if autoscalingRunnerSet.Annotations == nil { autoscalingRunnerSet.Annotations = map[string]string{} } diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index 07dfe7de..88b5faed 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -94,7 +94,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err := r.Get(ctx, req.NamespacedName, &ephemeralRunner); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - var original once[*v1alpha1.EphemeralRunner] + original := newOnce(ephemeralRunner.DeepCopy) if !ephemeralRunner.DeletionTimestamp.IsZero() { r.publishEphemeralRunnerPhaseMetric(&ephemeralRunner, "", log) @@ -117,7 +117,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } log.Info("Runner is cleaned up from the service, removing finalizer") - original.Do(ephemeralRunner.DeepCopy) + original.Do() controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) } if removeActionsFinalizer { @@ -147,7 +147,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ removeFinalizer := controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) if removeFinalizer { - original.Do(ephemeralRunner.DeepCopy) + original.Do() controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) } if removeFinalizer { @@ -181,12 +181,12 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ ephemeralRunnerFinalizerModified := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) if ephemeralRunnerFinalizerModified { - original.Do(ephemeralRunner.DeepCopy) + original.Do() controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) } ephemeralRunnerActionsFinalizerModified := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) if ephemeralRunnerActionsFinalizerModified { - original.Do(ephemeralRunner.DeepCopy) + original.Do() controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) } if ephemeralRunnerFinalizerModified || ephemeralRunnerActionsFinalizerModified { diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 0748f6ca..275ae7a7 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -80,7 +80,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.Get(ctx, req.NamespacedName, &ephemeralRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - var original once[*v1alpha1.EphemeralRunnerSet] + original := newOnce(ephemeralRunnerSet.DeepCopy) // Requested deletion does not need reconciled. if !ephemeralRunnerSet.DeletionTimestamp.IsZero() { @@ -111,7 +111,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R removeFinalizer := controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) if removeFinalizer { - original.Do(ephemeralRunnerSet.DeepCopy) + original.Do() controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) } if removeFinalizer { @@ -130,7 +130,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // Add finalizer if not present addFinalizer := !controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) if addFinalizer { - original.Do(ephemeralRunnerSet.DeepCopy) + original.Do() controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) } if addFinalizer { @@ -538,21 +538,19 @@ func (r *EphemeralRunnerSetReconciler) reconcileEphemeralRunnerSetProxySecret(ct } dataModified := !maps.EqualFunc(proxySecret.Data, desiredRunnerSetProxy.Data, bytes.Equal) - desiredLabels := r.filterAndMergeLabels(proxySecret.Labels, desiredRunnerSetProxy.Labels) - labelsModified := !maps.Equal(proxySecret.Labels, desiredLabels) - desiredAnnotations := r.mergeAnnotations(proxySecret.Annotations, desiredRunnerSetProxy.Annotations) - annotationsModified := !maps.Equal(proxySecret.Annotations, desiredAnnotations) - var original once[*corev1.Secret] + desiredLabels, labelsModified := r.mergeLabels(proxySecret.Labels, desiredRunnerSetProxy.Labels) + desiredAnnotations, annotationsModified := r.mergeAnnotations(proxySecret.Annotations, desiredRunnerSetProxy.Annotations) + original := newOnce(proxySecret.DeepCopy) if dataModified { - original.Do(proxySecret.DeepCopy) + original.Do() proxySecret.Data = desiredRunnerSetProxy.Data } if labelsModified { - original.Do(proxySecret.DeepCopy) + original.Do() proxySecret.Labels = desiredLabels } if annotationsModified { - original.Do(proxySecret.DeepCopy) + original.Do() proxySecret.Annotations = desiredAnnotations } if dataModified || labelsModified || annotationsModified { diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index 6f9942e6..b5b6ed14 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -1670,6 +1670,28 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { } } + waitForCachedActionableRevision := func(controller *EphemeralRunnerSetReconciler, key types.NamespacedName, specRevision, appliedRevision int64) { + Eventually(func(g Gomega) { + cached := new(v1alpha1.EphemeralRunnerSet) + g.Expect(controller.Get(ctx, key, cached)).To(Succeed()) + g.Expect(cached.Spec.ActionableRevision).To(Equal(specRevision)) + g.Expect(cached.Status.AppliedActionableRevision).To(Equal(appliedRevision)) + g.Expect(cached.Finalizers).To(ContainElement(EphemeralRunnerSetFinalizerName)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + } + + waitForCachedRunningRunners := func(controller *EphemeralRunnerSetReconciler, namespace, owner string, expected int) { + Eventually(func(g Gomega) { + runners := new(v1alpha1.EphemeralRunnerList) + g.Expect(controller.List(ctx, runners, client.InNamespace(namespace), client.MatchingFields{resourceOwnerKey: owner})).To(Succeed()) + state := newEphemeralRunnersByStates(runners) + g.Expect(state.running).To(HaveLen(expected)) + for _, runner := range state.running { + g.Expect(runner.Status.RunnerID).NotTo(BeZero()) + } + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + } + BeforeEach(func() { ctx = context.Background() autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) @@ -1904,6 +1926,9 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { err = k8sClient.Patch(ctx, specUpdated, client.MergeFrom(current)) Expect(err).NotTo(HaveOccurred()) + waitForCachedActionableRevision(controller, request.NamespacedName, 4, 3) + waitForCachedRunningRunners(controller, autoscalingNS.Name, ephemeralRunnerSet.Name, 1) + _, err = controller.Reconcile(ctx, request) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("remove failed")) diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index 3657cfaf..c5136d45 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -163,7 +163,7 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. ConfigSecretMetadata: autoscalingRunnerSet.Spec.ListenerConfigSecretMetadata, } - labels := b.filterAndMergeLabels(autoscalingRunnerSet.Labels, map[string]string{ + labels := b.propagateLabels(autoscalingRunnerSet.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingRunnerSet.Namespace, LabelKeyGitHubScaleSetName: autoscalingRunnerSet.Name, LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, @@ -178,8 +178,8 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. var annotations map[string]string if autoscalingRunnerSet.Spec.AutoscalingListenerMetadata != nil { - labels = b.filterAndMergeLabels(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Annotations, annotations) + labels, _ = b.mergeLabels(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Labels, labels) + annotations, _ = b.mergeAnnotations(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Annotations, annotations) } autoscalingListener := &v1alpha1.AutoscalingListener{ @@ -285,7 +285,7 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha var labels map[string]string if autoscalingListener.Spec.ConfigSecretMetadata != nil && len(autoscalingListener.Spec.ConfigSecretMetadata.Labels) > 0 { - labels = b.filterAndMergeLabels(autoscalingListener.Spec.ConfigSecretMetadata.Labels, nil) + labels = autoscalingListener.Spec.ConfigSecretMetadata.Labels } annotations := make(map[string]string) @@ -307,6 +307,7 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha Data: map[string][]byte{ "config.json": buf.Bytes(), }, + Type: corev1.SecretTypeOpaque, } if err := b.setControllerReference(autoscalingListener, desiredSecret); err != nil { @@ -593,27 +594,29 @@ func (b *ResourceBuilder) newScaleSetListenerServiceAccount(autoscalingListener return cached, nil } + labels := b.propagateLabels(autoscalingListener.Labels, map[string]string{ + LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, + LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, + }) + annotations := make(map[string]string) + + if autoscalingListener.Spec.ServiceAccountMetadata != nil { + labels, _ = b.mergeLabels(autoscalingListener.Spec.ServiceAccountMetadata.Labels, labels) + annotations, _ = b.mergeAnnotations(autoscalingListener.Spec.ServiceAccountMetadata.Annotations, annotations) + } + base := &corev1.ServiceAccount{ TypeMeta: metav1.TypeMeta{ APIVersion: corev1.SchemeGroupVersion.String(), Kind: "ServiceAccount", }, ObjectMeta: metav1.ObjectMeta{ - Name: autoscalingListener.Name, - Namespace: autoscalingListener.Namespace, - Labels: b.filterAndMergeLabels(autoscalingListener.Labels, map[string]string{ - LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, - LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, - }), - Annotations: make(map[string]string), + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Namespace, + Labels: labels, + Annotations: annotations, }, } - - if autoscalingListener.Spec.ServiceAccountMetadata != nil { - base.Labels = b.filterAndMergeLabels(autoscalingListener.Spec.ServiceAccountMetadata.Labels, base.Labels) - base.Annotations = b.mergeAnnotations(autoscalingListener.Spec.ServiceAccountMetadata.Annotations, base.Annotations) - } - if err := b.setControllerReference(autoscalingListener, base); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener service account: %w", err) } @@ -633,7 +636,7 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1. return cached } - labels := b.filterAndMergeLabels(autoscalingListener.Labels, map[string]string{ + labels := b.propagateLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, labelKeyListenerNamespace: autoscalingListener.Namespace, @@ -642,8 +645,8 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1. annotations := make(map[string]string) if autoscalingListener.Spec.RoleMetadata != nil { - labels = b.filterAndMergeLabels(autoscalingListener.Spec.RoleMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingListener.Spec.RoleMetadata.Annotations, nil) + labels, _ = b.mergeLabels(autoscalingListener.Spec.RoleMetadata.Labels, labels) + annotations = autoscalingListener.Spec.RoleMetadata.Annotations } newRole := &rbacv1.Role{ @@ -677,8 +680,9 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 } roleRef := rbacv1.RoleRef{ - Kind: "Role", - Name: listenerRole.Name, + APIGroup: rbacv1.GroupName, + Kind: "Role", + Name: listenerRole.Name, } subjects := []rbacv1.Subject{ @@ -689,7 +693,7 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 }, } - labels := b.filterAndMergeLabels(autoscalingListener.Labels, map[string]string{ + labels := b.propagateLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, labelKeyListenerNamespace: autoscalingListener.Namespace, @@ -698,7 +702,7 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 annotations := make(map[string]string) if autoscalingListener.Spec.RoleBindingMetadata != nil { - labels = b.filterAndMergeLabels(autoscalingListener.Spec.RoleBindingMetadata.Labels, labels) + labels, _ = b.mergeLabels(autoscalingListener.Spec.RoleBindingMetadata.Labels, labels) annotations = autoscalingListener.Spec.RoleBindingMetadata.Annotations } @@ -753,7 +757,7 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A EphemeralRunnerMetadata: autoscalingRunnerSet.Spec.EphemeralRunnerMetadata, } - labels := b.filterAndMergeLabels(autoscalingRunnerSet.Labels, map[string]string{ + labels := b.propagateLabels(autoscalingRunnerSet.Labels, map[string]string{ LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, LabelKeyKubernetesComponent: "runner-set", LabelKeyKubernetesVersion: autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], @@ -771,8 +775,8 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A } if autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata != nil { - labels = b.filterAndMergeLabels(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Annotations, annotations) + labels, _ = b.mergeLabels(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Labels, labels) + annotations, _ = b.mergeAnnotations(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Annotations, annotations) } newEphemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ @@ -809,6 +813,7 @@ func (b *ResourceBuilder) newAutoscalingListenerProxySecret(autoscalingListener Annotations: make(map[string]string, 1), }, Data: data, + Type: corev1.SecretTypeOpaque, } if err := b.setControllerReference(autoscalingListener, newProxySecret); err != nil { @@ -828,8 +833,8 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme annotations[AnnotationKeyPatchID] = strconv.Itoa(ephemeralRunnerSet.Spec.PatchID) if ephemeralRunnerSet.Spec.EphemeralRunnerMetadata != nil { - labels = b.filterAndMergeLabels(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Labels, labels) - annotations = b.mergeAnnotations(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Annotations, annotations) + labels, _ = b.mergeLabels(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Labels, labels) + annotations, _ = b.mergeAnnotations(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Annotations, annotations) } ephemeralRunner := &v1alpha1.EphemeralRunner{ @@ -925,7 +930,7 @@ func (b *ResourceBuilder) newEphemeralRunnerJitSecret(ephemeralRunner *v1alpha1. ) if ephemeralRunner.Spec.EphemeralRunnerConfigSecretMetadata != nil { - labels = b.filterAndMergeLabels(ephemeralRunner.Spec.EphemeralRunnerConfigSecretMetadata.Labels, nil) + labels = ephemeralRunner.Spec.EphemeralRunnerConfigSecretMetadata.Labels annotations = ephemeralRunner.Spec.EphemeralRunnerConfigSecretMetadata.Annotations } @@ -1051,39 +1056,67 @@ func trimLabelValue(val string) string { return strings.Trim(val, "-_.") } -func (b *ResourceBuilder) filterAndMergeLabels(base, overwrite map[string]string) map[string]string { +func (b *ResourceBuilder) shouldExcludePropagatedLabel(k, _ string) bool { + for _, prefix := range b.ExcludeLabelPropagationPrefixes { + if strings.HasPrefix(k, prefix) { + return true + } + } + return false +} + +// propagateLabels is responsible for filtering labels during propagation. +// It only makes sense when we are creating the resource derived from some other resource. +// Since the desired resource is cached, then we don't need to call this method every time. +func (b *ResourceBuilder) propagateLabels(base, overwrite map[string]string) map[string]string { if base == nil && overwrite == nil { return nil } - mergedLabels := make(map[string]string, len(base)) -base: + labels := make(map[string]string, len(base)+len(overwrite)) for k, v := range base { - for _, prefix := range b.ExcludeLabelPropagationPrefixes { - if strings.HasPrefix(k, prefix) { - continue base - } + if b.shouldExcludePropagatedLabel(k, v) { + continue } - mergedLabels[k] = v + labels[k] = v } + maps.Copy(labels, overwrite) -overwrite: - for k, v := range overwrite { - for _, prefix := range b.ExcludeLabelPropagationPrefixes { - if strings.HasPrefix(k, prefix) { - continue overwrite - } - } - mergedLabels[k] = v - } - - return mergedLabels -} - -func (b *ResourceBuilder) mergeAnnotations(base, overwrite map[string]string) map[string]string { - if base == nil && overwrite == nil { + if len(labels) == 0 { return nil } - maps.Copy(base, overwrite) - return base + return labels +} + +func (b *ResourceBuilder) mergeLabels(base, overwrite map[string]string) (map[string]string, bool) { + return mergeMaps(base, overwrite) +} + +func (b *ResourceBuilder) mergeAnnotations(base, overwrite map[string]string) (map[string]string, bool) { + return mergeMaps(base, overwrite) +} + +func mergeMaps[M ~map[K]V, K comparable, V comparable](base M, overwrite M) (M, bool) { + if len(overwrite) == 0 { + return base, false + } + + if containsMapEntries(base, overwrite) { + return base, false + } + + merged := make(M, len(base)+len(overwrite)) + maps.Copy(merged, base) + maps.Copy(merged, overwrite) + return merged, true +} + +func containsMapEntries[M ~map[K]V, K comparable, V comparable](base M, entries M) bool { + for k, v := range entries { + current, ok := base[k] + if !ok || current != v { + return false + } + } + return true } diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index 9e91790d..3a3ad4cf 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -157,6 +158,7 @@ func TestMetadataPropagation(t *testing.T) { assert.Equal(t, "listener-role-annotation", listenerRole.Annotations["test.com/listener-role-annotation"]) listenerRoleBinding := b.newScaleSetListenerRoleBinding(listener, listenerRole, listenerServiceAccount) + assert.Equal(t, rbacv1.GroupName, listenerRoleBinding.RoleRef.APIGroup) assert.Equal(t, "listener-role-binding-label", listenerRoleBinding.Labels["test.com/listener-role-binding-label"]) assert.Equal(t, "listener-role-binding-annotation", listenerRoleBinding.Annotations["test.com/listener-role-binding-annotation"]) @@ -231,6 +233,75 @@ func TestEphemeralRunnerSetProxySecretMetadata(t *testing.T) { assert.NotContains(t, proxySecret.Annotations, "actions.github.com/integrity-hash") } +func TestMergeMaps(t *testing.T) { + t.Run("equal overwrite is not modified", func(t *testing.T) { + base := map[string]string{ + "a": "1", + "b": "2", + } + overwrite := map[string]string{ + "a": "1", + "b": "2", + } + + merged, modified := mergeMaps(base, overwrite) + + require.False(t, modified) + merged["a"] = "changed" + assert.Equal(t, "changed", base["a"]) + }) + + t.Run("base superset is not modified", func(t *testing.T) { + base := map[string]string{ + "a": "1", + "b": "2", + "extra": "base-only", + } + overwrite := map[string]string{ + "a": "1", + "b": "2", + } + + merged, modified := mergeMaps(base, overwrite) + + require.False(t, modified) + merged["a"] = "changed" + assert.Equal(t, "changed", base["a"]) + }) + + t.Run("missing overwrite key is modified", func(t *testing.T) { + base := map[string]string{ + "a": "1", + } + overwrite := map[string]string{ + "a": "1", + "b": "2", + } + + merged, modified := mergeMaps(base, overwrite) + + require.True(t, modified) + assert.Equal(t, map[string]string{"a": "1", "b": "2"}, merged) + assert.NotContains(t, base, "b") + }) + + t.Run("different overwrite value is modified", func(t *testing.T) { + base := map[string]string{ + "a": "1", + "b": "2", + } + overwrite := map[string]string{ + "a": "updated", + } + + merged, modified := mergeMaps(base, overwrite) + + require.True(t, modified) + assert.Equal(t, map[string]string{"a": "updated", "b": "2"}, merged) + assert.Equal(t, "1", base["a"]) + }) +} + func TestGitHubURLTrimLabelValues(t *testing.T) { enterprise := strings.Repeat("a", 64) organization := strings.Repeat("b", 64) diff --git a/controllers/actions.github.com/utils.go b/controllers/actions.github.com/utils.go index c17aba6d..482c95d4 100644 --- a/controllers/actions.github.com/utils.go +++ b/controllers/actions.github.com/utils.go @@ -16,21 +16,31 @@ func FilterLabels(labels map[string]string, filter string) map[string]string { type once[T client.Object] struct { value T - fn func(T) *T + fn func() T done bool } -func (o *once[T]) Do(f func() T) T { +func newOnce[T client.Object](fn func() T) *once[T] { + return &once[T]{ + fn: fn, + } +} + +func (o *once[T]) Do() T { if !o.done { - o.value = f() + o.value = o.fn() o.done = true } return o.value } func (o *once[T]) Get() T { - if !o.done { + if !o.Called() { panic("not done") } return o.value } + +func (o *once[T]) Called() bool { + return o.done +}