Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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 <[email protected]>
  • Loading branch information
puretension committed Sep 19, 2025
commit 55ec5568109b4f9cb2650294ea1caf4efa3ae4e6
3 changes: 2 additions & 1 deletion cmd/argocd/commands/admin/notifications.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Claude @puretension!

Could you not modify the existing GetFactorySettingsForCLI instead, since this is the only place where that function is used? Another option is to revert #23424, but it's perhaps more clear with a deferred function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @blakepettersson!
You're absolutely right! I've modified the existing GetFactorySettingsForCLI
function to use the deferred pattern instead of creating a duplicate function.
This approach is much cleaner and eliminates the code duplication while still
solving the initialization issue. I also updated the test to match the changes.

Thanks for pointing this out. it's a much better solution!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blakepettersson Thanks for the review!
I've addressed the feedback and all tests are now passing.
Ready for another look!

func(clientConfig clientcmd.ClientConfig) {
k8sCfg, err := clientConfig.ClientConfig()
if err != nil {
Expand Down
19 changes: 19 additions & 0 deletions util/notification/settings/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
95 changes: 15 additions & 80 deletions util/notification/settings/settings_test.go
Original file line number Diff line number Diff line change
@@ -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"

Check failure on line 9 in util/notification/settings/settings_test.go

View workflow job for this annotation

GitHub Actions / Lint Go code

File is not properly formatted (gofumpt)
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, &notificationsCm, &notificationsSecret)
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)

Check failure on line 28 in util/notification/settings/settings_test.go

View workflow job for this annotation

GitHub Actions / Lint Go code

require-error: for error assertions use require (testifylint)
assert.Contains(t, err.Error(), "argocdService is not initialized")
}
Loading