chore: Address comments in PR #854 (#886)

Signed-off-by: Ishita Sequeira <ishiseq29@gmail.com>

3 files changed, 65 insertions, 9 deletions

diff --git a/pkg/argocd/argocd.go b/pkg/argocd/argocd.go
index 9846e7e..63ac7a7 100644
--- a/pkg/argocd/argocd.go
+++ b/pkg/argocd/argocd.go
@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/argoproj-labs/argocd-image-updater/pkg/common"
+	"github.com/argoproj-labs/argocd-image-updater/pkg/env"
 	"github.com/argoproj-labs/argocd-image-updater/pkg/image"
 	"github.com/argoproj-labs/argocd-image-updater/pkg/kube"
 	"github.com/argoproj-labs/argocd-image-updater/pkg/log"
@@ -31,7 +32,7 @@ func (client *k8sClient) GetApplication(ctx context.Context, appName string) (*v
 	log.Debugf("Getting application %s across all namespaces", appName)
 
 	// List all applications across all namespaces (using empty labelSelector)
-	appList, err := client.ListApplications("")
+	appList, err := client.ListApplications(v1.NamespaceAll)
 	if err != nil {
 		return nil, fmt.Errorf("error listing applications: %w", err)
 	}
@@ -44,7 +45,7 @@ func (client *k8sClient) GetApplication(ctx context.Context, appName string) (*v
 	}
 
 	// Retrieve the application in the specified namespace
-	return client.kubeClient.ApplicationsClientset.ArgoprojV1alpha1().Applications(app.Namespace).Get(ctx, app.Name, v1.GetOptions{})
+	return app, nil
 }
 
 // ListApplications lists all applications across all namespaces.
@@ -85,13 +86,7 @@ func (client *k8sClient) UpdateSpec(ctx context.Context, spec *application.Appli
 	const baseDelay = 100 * time.Millisecond // Initial delay before retrying
 
 	// Allow overriding max retries for testing purposes
-	maxRetries := defaultMaxRetries
-	if overrideRetries, ok := os.LookupEnv("OVERRIDE_MAX_RETRIES"); ok {
-		var retries int
-		if _, err := fmt.Sscanf(overrideRetries, "%d", &retries); err == nil {
-			maxRetries = retries
-		}
-	}
+	maxRetries := env.ParseNumFromEnv("OVERRIDE_MAX_RETRIES", defaultMaxRetries, 0, 100)
 
 	for attempts := 0; attempts < maxRetries; attempts++ {
 		app, err := client.GetApplication(ctx, spec.GetName())
diff --git a/pkg/env/env.go b/pkg/env/env.go
index 966dc51..e780067 100644
--- a/pkg/env/env.go
+++ b/pkg/env/env.go
@@ -1,8 +1,12 @@
 package env
 
 import (
+	"math"
 	"os"
+	"strconv"
 	"strings"
+
+	"github.com/argoproj-labs/argocd-image-updater/pkg/log"
 )
 
 // Package env provides some utility functions to interact with the environment
@@ -30,3 +34,33 @@ func GetStringVal(envVar string, defaultValue string) string {
 		return defaultValue
 	}
 }
+
+// Helper function to parse a number from an environment variable. Returns a
+// default if env is not set, is not parseable to a number, exceeds max (if
+// max is greater than 0) or is less than min.
+//
+// nolint:unparam
+func ParseNumFromEnv(env string, defaultValue, min, max int) int {
+	str := os.Getenv(env)
+	if str == "" {
+		return defaultValue
+	}
+	num, err := strconv.ParseInt(str, 10, 0)
+	if err != nil {
+		log.Warnf("Could not parse '%s' as a number from environment %s", str, env)
+		return defaultValue
+	}
+	if num > math.MaxInt || num < math.MinInt {
+		log.Warnf("Value in %s is %d is outside of the min and max %d allowed values. Using default %d", env, num, min, defaultValue)
+		return defaultValue
+	}
+	if int(num) < min {
+		log.Warnf("Value in %s is %d, which is less than minimum %d allowed", env, num, min)
+		return defaultValue
+	}
+	if int(num) > max {
+		log.Warnf("Value in %s is %d, which is greater than maximum %d allowed", env, num, max)
+		return defaultValue
+	}
+	return int(num)
+}
diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go
index fb83580..41ccaf9 100644
--- a/pkg/env/env_test.go
+++ b/pkg/env/env_test.go
@@ -36,3 +36,30 @@ func Test_GetStringVal(t *testing.T) {
 		assert.Equal(t, "invalid", GetStringVal("TEST_STRING_VAL", "invalid"))
 	})
 }
+
+func Test_ParseNumFromEnv(t *testing.T) {
+	t.Run("Get number from existing env var within range", func(t *testing.T) {
+		_ = os.Setenv("TEST_NUM_VAL", "5")
+		defer os.Setenv("TEST_NUM_VAL", "")
+		assert.Equal(t, 5, ParseNumFromEnv("TEST_NUM_VAL", 0, 1, 10))
+	})
+	t.Run("Get default value from non-existing env var", func(t *testing.T) {
+		_ = os.Setenv("TEST_NUM_VAL", "")
+		assert.Equal(t, 10, ParseNumFromEnv("TEST_NUM_VAL", 10, 1, 20))
+	})
+	t.Run("Get default value from env var with non-numeric value", func(t *testing.T) {
+		_ = os.Setenv("TEST_NUM_VAL", "abc")
+		defer os.Setenv("TEST_NUM_VAL", "")
+		assert.Equal(t, 10, ParseNumFromEnv("TEST_NUM_VAL", 10, 1, 20))
+	})
+	t.Run("Get default value from env var with value less than min", func(t *testing.T) {
+		_ = os.Setenv("TEST_NUM_VAL", "0")
+		defer os.Setenv("TEST_NUM_VAL", "")
+		assert.Equal(t, 10, ParseNumFromEnv("TEST_NUM_VAL", 10, 1, 20))
+	})
+	t.Run("Get default value from env var with value greater than max", func(t *testing.T) {
+		_ = os.Setenv("TEST_NUM_VAL", "30")
+		defer os.Setenv("TEST_NUM_VAL", "")
+		assert.Equal(t, 10, ParseNumFromEnv("TEST_NUM_VAL", 10, 1, 20))
+	})
+}