fix: don't log non-errors as "Non-transient error: <nil>". Fixes #13881…

… (#13917) Signed-off-by: Mason Malone <[email protected]>
argoproj · Nov 24, 2024 · 6b221f4 · 6b221f4
1 parent f2159dc
commit 6b221f4
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 2 deletions.
diff --git a/test/e2e/signals_test.go b/test/e2e/signals_test.go
@@ -150,7 +150,7 @@ func (s *SignalsSuite) TestSignaledContainerSet() {
 		Workflow("@testdata/signaled-container-set-workflow.yaml").
 		When().
 		SubmitWorkflow().
-		WaitForWorkflow().
+		WaitForWorkflow(killDuration).
 		Then().
 		ExpectWorkflow(func(t *testing.T, metadata *metav1.ObjectMeta, status *wfv1.WorkflowStatus) {
 			assert.Equal(t, wfv1.WorkflowFailed, status.Phase)

diff --git a/util/errors/errors.go b/util/errors/errors.go
@@ -26,7 +26,7 @@ func IgnoreContainerNotFoundErr(err error) error {
 // IsTransientErr reports whether the error is transient and logs it.
 func IsTransientErr(err error) bool {
 	isTransient := IsTransientErrQuiet(err)
-	if !isTransient {
+	if err != nil && !isTransient {
 		log.Warnf("Non-transient error: %v", err)
 	}
 	return isTransient

diff --git a/util/errors/errors_test.go b/util/errors/errors_test.go
@@ -8,6 +8,8 @@ import (
 	"os/exec"
 	"testing"
 
+	log "github.com/sirupsen/logrus"
+	logtest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	apierr "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -50,12 +52,19 @@ var (
 const transientEnvVarKey = "TRANSIENT_ERROR_PATTERN"
 
 func TestIsTransientErr(t *testing.T) {
+	hook := &logtest.Hook{}
+	log.AddHook(hook)
+	defer log.StandardLogger().ReplaceHooks(nil)
+
 	t.Run("Nil", func(t *testing.T) {
 		assert.False(t, IsTransientErr(nil))
+		assert.Nil(t, hook.LastEntry())
 	})
 	t.Run("ResourceQuotaConflictErr", func(t *testing.T) {
 		assert.False(t, IsTransientErr(apierr.NewConflict(schema.GroupResource{}, "", nil)))
+		assert.Contains(t, hook.LastEntry().Message, "Non-transient error:")
 		assert.True(t, IsTransientErr(apierr.NewConflict(schema.GroupResource{Group: "v1", Resource: "resourcequotas"}, "", nil)))
+		assert.Contains(t, hook.LastEntry().Message, "Transient error:")
 	})
 	t.Run("ResourceQuotaTimeoutErr", func(t *testing.T) {
 		assert.False(t, IsTransientErr(apierr.NewInternalError(errors.New(""))))