From 55ec5568109b4f9cb2650294ea1caf4efa3ae4e6 Mon Sep 17 00:00:00 2001 From: puretension Date: Fri, 19 Sep 2025 22:53:15 +0900 Subject: [PATCH 1/4] fix: resolve argocdService initialization issue in notifications CLI The notifications CLI commands were failing with 'argocdService is not initialized' error in v3.1.0 due to incorrect initialization order. This fix introduces GetFactorySettingsDeferred that uses a closure to defer service access until it's actually needed, ensuring the service is properly initialized when accessed. Changes: - Add GetFactorySettingsDeferred function with simplified naming - Add unit test to verify error handling for uninitialized service - Update notifications command to use deferred initialization pattern Fixes #24196 Signed-off-by: puretension --- cmd/argocd/commands/admin/notifications.go | 3 +- util/notification/settings/settings.go | 19 +++++ util/notification/settings/settings_test.go | 95 ++++----------------- 3 files changed, 36 insertions(+), 81 deletions(-) diff --git a/cmd/argocd/commands/admin/notifications.go b/cmd/argocd/commands/admin/notifications.go index 39fd81b9ccb8b..f3ea6a39f3163 100644 --- a/cmd/argocd/commands/admin/notifications.go +++ b/cmd/argocd/commands/admin/notifications.go @@ -30,11 +30,12 @@ func NewNotificationsCommand() *cobra.Command { ) var argocdService service.Service + toolsCommand := cmd.NewToolsCommand( "notifications", "argocd admin notifications", applications, - settings.GetFactorySettingsForCLI(argocdService, "argocd-notifications-secret", "argocd-notifications-cm", false), + settings.GetFactorySettingsDeferred(func() service.Service { return argocdService }, "argocd-notifications-secret", "argocd-notifications-cm", false), func(clientConfig clientcmd.ClientConfig) { k8sCfg, err := clientConfig.ClientConfig() if err != nil { diff --git a/util/notification/settings/settings.go b/util/notification/settings/settings.go index 9ff3c7675cede..27ca413efa5c1 100644 --- a/util/notification/settings/settings.go +++ b/util/notification/settings/settings.go @@ -45,6 +45,25 @@ func GetFactorySettingsForCLI(argocdService service.Service, secretName, configM } } +// GetFactorySettingsDeferred allows deferred service initialization for CLI commands. +func GetFactorySettingsDeferred(serviceGetter func() service.Service, secretName, configMapName string, selfServiceNotificationEnabled bool) api.Settings { + return api.Settings{ + SecretName: secretName, + ConfigMapName: configMapName, + InitGetVars: func(cfg *api.Config, configMap *corev1.ConfigMap, secret *corev1.Secret) (api.GetVars, error) { + argocdService := serviceGetter() + if argocdService == nil { + return nil, errors.New("argocdService is not initialized") + } + + if selfServiceNotificationEnabled { + return initGetVarsWithoutSecret(argocdService, cfg, configMap, secret) + } + return initGetVars(argocdService, cfg, configMap, secret) + }, + } +} + func getContext(cfg *api.Config, configMap *corev1.ConfigMap, secret *corev1.Secret) (map[string]string, error) { context := map[string]string{} if contextYaml, ok := configMap.Data["context"]; ok { diff --git a/util/notification/settings/settings_test.go b/util/notification/settings/settings_test.go index 35c91ac977b07..5ef82108f0f36 100644 --- a/util/notification/settings/settings_test.go +++ b/util/notification/settings/settings_test.go @@ -1,95 +1,30 @@ package settings import ( - "fmt" "testing" - "github.com/argoproj/notifications-engine/pkg/api" - "github.com/argoproj/notifications-engine/pkg/services" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes/fake" - "github.com/argoproj/argo-cd/v3/reposerver/apiclient/mocks" + "github.com/argoproj/notifications-engine/pkg/api" service "github.com/argoproj/argo-cd/v3/util/notification/argocd" ) -const ( - testNamespace = "default" - testContextKey = "test-context-key" - testContextKeyValue = "test-context-key-value" -) +func TestGetFactorySettingsDeferred_ServiceNotInitialized(t *testing.T) { + var argocdService service.Service // nil service -func TestInitGetVars(t *testing.T) { - notificationsCm := corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testNamespace, - Name: "argocd-notifications-cm", - }, - Data: map[string]string{ - "context": fmt.Sprintf("%s: %s", testContextKey, testContextKeyValue), - "service.webhook.test": "url: https://test.example.com", - "template.app-created": "email:\n subject: Application {{.app.metadata.name}} has been created.\nmessage: Application {{.app.metadata.name}} has been created.\nteams:\n title: Application {{.app.metadata.name}} has been created.\n", - "trigger.on-created": "- description: Application is created.\n oncePer: app.metadata.name\n send:\n - app-created\n when: \"true\"\n", - }, - } - notificationsSecret := corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "argocd-notifications-secret", - Namespace: testNamespace, - }, - Data: map[string][]byte{ - "notification-secret": []byte("secret-value"), - }, - } - kubeclientset := fake.NewClientset(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: testNamespace, - Name: "argocd-notifications-cm", - }, - Data: notificationsCm.Data, - }, - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "argocd-notifications-secret", - Namespace: testNamespace, - }, - Data: notificationsSecret.Data, - }) - mockRepoClient := &mocks.Clientset{RepoServerServiceClient: &mocks.RepoServerServiceClient{}} - argocdService, err := service.NewArgoCDService(kubeclientset, testNamespace, mockRepoClient) - require.NoError(t, err) - defer argocdService.Close() - config := api.Config{} - testDestination := services.Destination{ - Service: "webhook", - } - emptyAppData := map[string]any{} + settings := GetFactorySettingsDeferred( + func() service.Service { return argocdService }, + "test-secret", + "test-configmap", + false, + ) - varsProvider, _ := initGetVars(argocdService, &config, ¬ificationsCm, ¬ificationsSecret) + cfg := &api.Config{} + configMap := &corev1.ConfigMap{} + secret := &corev1.Secret{} - t.Run("Vars provider serves Application data on app key", func(t *testing.T) { - appData := map[string]any{ - "name": "app-name", - } - result := varsProvider(appData, testDestination) - assert.NotNil(t, t, result["app"]) - assert.Equal(t, result["app"], appData) - }) - t.Run("Vars provider serves notification context data on context key", func(t *testing.T) { - expectedContext := map[string]string{ - testContextKey: testContextKeyValue, - "notificationType": testDestination.Service, - } - result := varsProvider(emptyAppData, testDestination) - assert.NotNil(t, result["context"]) - assert.Equal(t, expectedContext, result["context"]) - }) - t.Run("Vars provider serves notification secrets on secrets key", func(t *testing.T) { - result := varsProvider(emptyAppData, testDestination) - assert.NotNil(t, result["secrets"]) - assert.Equal(t, result["secrets"], notificationsSecret.Data) - }) + _, err := settings.InitGetVars(cfg, configMap, secret) + assert.Error(t, err) + assert.Contains(t, err.Error(), "argocdService is not initialized") } From b9042a1a9be7bf6b2c17610c8229c309ece7c6ea Mon Sep 17 00:00:00 2001 From: puretension Date: Sat, 20 Sep 2025 00:05:26 +0900 Subject: [PATCH 2/4] fix: resolve golangci-lint issues in settings_test.go - Fix gofumpt formatting by reordering imports - Replace assert with require for error assertions per testifylint Signed-off-by: puretension --- util/notification/settings/settings_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/util/notification/settings/settings_test.go b/util/notification/settings/settings_test.go index 5ef82108f0f36..dde28f528de95 100644 --- a/util/notification/settings/settings_test.go +++ b/util/notification/settings/settings_test.go @@ -3,11 +3,10 @@ package settings import ( "testing" - "github.com/stretchr/testify/assert" - corev1 "k8s.io/api/core/v1" - "github.com/argoproj/notifications-engine/pkg/api" service "github.com/argoproj/argo-cd/v3/util/notification/argocd" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" ) func TestGetFactorySettingsDeferred_ServiceNotInitialized(t *testing.T) { @@ -25,6 +24,6 @@ func TestGetFactorySettingsDeferred_ServiceNotInitialized(t *testing.T) { secret := &corev1.Secret{} _, err := settings.InitGetVars(cfg, configMap, secret) - assert.Error(t, err) - assert.Contains(t, err.Error(), "argocdService is not initialized") + require.Error(t, err) + require.Contains(t, err.Error(), "argocdService is not initialized") } From 915a07147c6845394fa5e5e94bd66bae3405f782 Mon Sep 17 00:00:00 2001 From: puretension Date: Sat, 20 Sep 2025 20:53:51 +0900 Subject: [PATCH 3/4] fix: modify GetFactorySettingsForCLI to use deferred pattern As requested by maintainer, modified the existing GetFactorySettingsForCLI function to use the deferred pattern instead of creating a duplicate function. This eliminates code duplication while solving the initialization issue. Fixes #24196 Signed-off-by: puretension --- cmd/argocd/commands/admin/notifications.go | 2 +- util/notification/settings/settings.go | 20 +------------------- 2 files changed, 2 insertions(+), 20 deletions(-) diff --git a/cmd/argocd/commands/admin/notifications.go b/cmd/argocd/commands/admin/notifications.go index f3ea6a39f3163..337190c287160 100644 --- a/cmd/argocd/commands/admin/notifications.go +++ b/cmd/argocd/commands/admin/notifications.go @@ -35,7 +35,7 @@ func NewNotificationsCommand() *cobra.Command { "notifications", "argocd admin notifications", applications, - settings.GetFactorySettingsDeferred(func() service.Service { return argocdService }, "argocd-notifications-secret", "argocd-notifications-cm", false), + settings.GetFactorySettingsForCLI(func() service.Service { return argocdService }, "argocd-notifications-secret", "argocd-notifications-cm", false), func(clientConfig clientcmd.ClientConfig) { k8sCfg, err := clientConfig.ClientConfig() if err != nil { diff --git a/util/notification/settings/settings.go b/util/notification/settings/settings.go index 27ca413efa5c1..9aba0085146f7 100644 --- a/util/notification/settings/settings.go +++ b/util/notification/settings/settings.go @@ -28,25 +28,7 @@ func GetFactorySettings(argocdService service.Service, secretName, configMapName } // GetFactorySettingsForCLI allows the initialization of argocdService to be deferred until it is used, when InitGetVars is called. -func GetFactorySettingsForCLI(argocdService service.Service, secretName, configMapName string, selfServiceNotificationEnabled bool) api.Settings { - return api.Settings{ - SecretName: secretName, - ConfigMapName: configMapName, - InitGetVars: func(cfg *api.Config, configMap *corev1.ConfigMap, secret *corev1.Secret) (api.GetVars, error) { - if argocdService == nil { - return nil, errors.New("argocdService is not initialized") - } - - if selfServiceNotificationEnabled { - return initGetVarsWithoutSecret(argocdService, cfg, configMap, secret) - } - return initGetVars(argocdService, cfg, configMap, secret) - }, - } -} - -// GetFactorySettingsDeferred allows deferred service initialization for CLI commands. -func GetFactorySettingsDeferred(serviceGetter func() service.Service, secretName, configMapName string, selfServiceNotificationEnabled bool) api.Settings { +func GetFactorySettingsForCLI(serviceGetter func() service.Service, secretName, configMapName string, selfServiceNotificationEnabled bool) api.Settings { return api.Settings{ SecretName: secretName, ConfigMapName: configMapName, From 15a50cc9169f3ebc206c9305335b7ce194a83a64 Mon Sep 17 00:00:00 2001 From: puretension Date: Sat, 20 Sep 2025 21:04:10 +0900 Subject: [PATCH 4/4] revert: restore original settings_test.go As requested by maintainer, keep existing tests unchanged. Only modify the core functionality without touching test files. Fixes #24196 Signed-off-by: puretension --- util/notification/settings/settings_test.go | 96 +++++++++++++++++---- 1 file changed, 81 insertions(+), 15 deletions(-) diff --git a/util/notification/settings/settings_test.go b/util/notification/settings/settings_test.go index dde28f528de95..35c91ac977b07 100644 --- a/util/notification/settings/settings_test.go +++ b/util/notification/settings/settings_test.go @@ -1,29 +1,95 @@ package settings import ( + "fmt" "testing" "github.com/argoproj/notifications-engine/pkg/api" - service "github.com/argoproj/argo-cd/v3/util/notification/argocd" + "github.com/argoproj/notifications-engine/pkg/services" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + + "github.com/argoproj/argo-cd/v3/reposerver/apiclient/mocks" + service "github.com/argoproj/argo-cd/v3/util/notification/argocd" ) -func TestGetFactorySettingsDeferred_ServiceNotInitialized(t *testing.T) { - var argocdService service.Service // nil service +const ( + testNamespace = "default" + testContextKey = "test-context-key" + testContextKeyValue = "test-context-key-value" +) - settings := GetFactorySettingsDeferred( - func() service.Service { return argocdService }, - "test-secret", - "test-configmap", - false, - ) +func TestInitGetVars(t *testing.T) { + notificationsCm := corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: "argocd-notifications-cm", + }, + Data: map[string]string{ + "context": fmt.Sprintf("%s: %s", testContextKey, testContextKeyValue), + "service.webhook.test": "url: https://test.example.com", + "template.app-created": "email:\n subject: Application {{.app.metadata.name}} has been created.\nmessage: Application {{.app.metadata.name}} has been created.\nteams:\n title: Application {{.app.metadata.name}} has been created.\n", + "trigger.on-created": "- description: Application is created.\n oncePer: app.metadata.name\n send:\n - app-created\n when: \"true\"\n", + }, + } + notificationsSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argocd-notifications-secret", + Namespace: testNamespace, + }, + Data: map[string][]byte{ + "notification-secret": []byte("secret-value"), + }, + } + kubeclientset := fake.NewClientset(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: "argocd-notifications-cm", + }, + Data: notificationsCm.Data, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argocd-notifications-secret", + Namespace: testNamespace, + }, + Data: notificationsSecret.Data, + }) + mockRepoClient := &mocks.Clientset{RepoServerServiceClient: &mocks.RepoServerServiceClient{}} + argocdService, err := service.NewArgoCDService(kubeclientset, testNamespace, mockRepoClient) + require.NoError(t, err) + defer argocdService.Close() + config := api.Config{} + testDestination := services.Destination{ + Service: "webhook", + } + emptyAppData := map[string]any{} - cfg := &api.Config{} - configMap := &corev1.ConfigMap{} - secret := &corev1.Secret{} + varsProvider, _ := initGetVars(argocdService, &config, ¬ificationsCm, ¬ificationsSecret) - _, err := settings.InitGetVars(cfg, configMap, secret) - require.Error(t, err) - require.Contains(t, err.Error(), "argocdService is not initialized") + t.Run("Vars provider serves Application data on app key", func(t *testing.T) { + appData := map[string]any{ + "name": "app-name", + } + result := varsProvider(appData, testDestination) + assert.NotNil(t, t, result["app"]) + assert.Equal(t, result["app"], appData) + }) + t.Run("Vars provider serves notification context data on context key", func(t *testing.T) { + expectedContext := map[string]string{ + testContextKey: testContextKeyValue, + "notificationType": testDestination.Service, + } + result := varsProvider(emptyAppData, testDestination) + assert.NotNil(t, result["context"]) + assert.Equal(t, expectedContext, result["context"]) + }) + t.Run("Vars provider serves notification secrets on secrets key", func(t *testing.T) { + result := varsProvider(emptyAppData, testDestination) + assert.NotNil(t, result["secrets"]) + assert.Equal(t, result["secrets"], notificationsSecret.Data) + }) }