Fix scheduling week N (#92)

We incorrectly returned an error in the helper function `CheckIsoWeek` if ISO week `N` did not match current week. This PR fixes the return value to `false` and the error to nil as expected.

Author: bastjan

controllers/upgradeconfig_controller.go

@@ -21,6 +21,7 @@ import (
 
 	managedupgradev1beta1 "github.com/appuio/openshift-upgrade-controller/api/v1beta1"
 	"github.com/appuio/openshift-upgrade-controller/pkg/clusterversion"
+	"github.com/appuio/openshift-upgrade-controller/pkg/scheduleutils"
 )
 
 const (
@@ -287,7 +288,7 @@ func calcNextRun(earliest time.Time, sched cron.Schedule, schedISOWeek string) (
 	nextRun := sched.Next(earliest)
 	// if the next run is more than 1000 runs away, we assume that the cron schedule is invalid as a safe guard
 	for i := 0; i < 1000; i++ {
-		isoWeekOK, err := checkIsoWeek(nextRun, schedISOWeek)
+		isoWeekOK, err := scheduleutils.CheckIsoWeek(nextRun, schedISOWeek)
 		if err != nil {
 			return time.Time{}, err
 		}
@@ -298,25 +299,3 @@ func calcNextRun(earliest time.Time, sched cron.Schedule, schedISOWeek string) (
 	}
 	return time.Time{}, fmt.Errorf("could not find next run, max time: %s", nextRun)
 }
-
-// checkIsoWeek checks if the given time is in the given iso week.
-// The iso week can be one of the following:
-// - "": every iso week
-// - "@even": every even iso week
-// - "@odd": every odd iso week
-// - "<N>": every iso week N
-func checkIsoWeek(t time.Time, schedISOWeek string) (bool, error) {
-	_, iw := t.ISOWeek()
-	switch schedISOWeek {
-	case "":
-		return true, nil
-	case "@even":
-		return iw%2 == 0, nil
-	case "@odd":
-		return iw%2 == 1, nil
-	case strconv.Itoa(iw):
-		return true, nil
-	default:
-		return false, fmt.Errorf("unknown iso week: %s", schedISOWeek)
-	}
-}

pkg/scheduleutils/scheduleutils.go

@@ -0,0 +1,31 @@
+package scheduleutils
+
+import (
+	"fmt"
+	"strconv"
+	"time"
+)
+
+// CheckIsoWeek checks if the given time is in the given iso week.
+// The iso week can be one of the following:
+// - "": every iso week
+// - "@even": every even iso week
+// - "@odd": every odd iso week
+// - "<N>": every iso week N
+func CheckIsoWeek(t time.Time, schedISOWeek string) (bool, error) {
+	_, iw := t.ISOWeek()
+	switch schedISOWeek {
+	case "":
+		return true, nil
+	case "@even":
+		return iw%2 == 0, nil
+	case "@odd":
+		return iw%2 == 1, nil
+	}
+
+	nw, err := strconv.ParseInt(schedISOWeek, 10, 64)
+	if err == nil {
+		return nw == int64(iw), nil
+	}
+	return false, fmt.Errorf("unknown iso week: %s", schedISOWeek)
+}

pkg/scheduleutils/scheduleutils_test.go

@@ -0,0 +1,77 @@
+package scheduleutils
+
+import (
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestCheckIsoWeek(t *testing.T) {
+	tc := []struct {
+		t            time.Time
+		schedISOWeek string
+		expected     bool
+		expectedErr  error
+	}{
+		{
+			t:            time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC),
+			schedISOWeek: "",
+			expected:     true,
+		},
+		{
+			t:            time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC),
+			schedISOWeek: "@even",
+			expected:     false,
+		},
+		{
+			t:            time.Date(2021, 1, 13, 0, 0, 0, 0, time.UTC),
+			schedISOWeek: "@even",
+			expected:     true,
+		},
+		{
+			t:            time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC),
+			schedISOWeek: "@odd",
+			expected:     true,
+		},
+		{
+			t:            time.Date(2021, 1, 13, 0, 0, 0, 0, time.UTC),
+			schedISOWeek: "@odd",
+			expected:     false,
+		},
+		{
+			t:            time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC),
+			schedISOWeek: "53",
+			expected:     true,
+		},
+		{
+			t:            time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC),
+			schedISOWeek: "1",
+			expected:     false,
+		},
+		{
+			t:            time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC),
+			schedISOWeek: "invalid",
+			expectedErr:  fmt.Errorf("unknown iso week: invalid"),
+		},
+	}
+
+	for _, c := range tc {
+		_, iw := c.t.ISOWeek()
+		t.Run(fmt.Sprintf("sched: %s, iso week from time: %d", c.schedISOWeek, iw), func(t *testing.T) {
+			got, err := CheckIsoWeek(c.t, c.schedISOWeek)
+			if err != nil {
+				if c.expectedErr == nil {
+					t.Fatalf("unexpected error: %v", err)
+				}
+				require.Equal(t, c.expectedErr.Error(), err.Error())
+				return
+			}
+			if c.expectedErr != nil {
+				t.Fatalf("expected error %q, got nil", c.expectedErr)
+			}
+			require.Equal(t, c.expected, got)
+		})
+	}
+}