diff --git a/controllers/nstemplatetier/nstemplatetier_controller.go b/controllers/nstemplatetier/nstemplatetier_controller.go index d58ca6cdb..2aa4754c8 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller.go +++ b/controllers/nstemplatetier/nstemplatetier_controller.go @@ -5,13 +5,11 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/toolchainconfig" - "github.com/codeready-toolchain/toolchain-common/pkg/hash" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/log" errs "github.com/pkg/errors" "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -20,12 +18,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -// ---------------------------------------------------------------------------------------------------------------------------- -// NSTemplateTier Controller Reconciler: -// . in case of a new NSTemplateTier update to process: -// .. inserts a new record in the `status.updates` history -// ---------------------------------------------------------------------------------------------------------------------------- - // SetupWithManager sets up the controller with the Manager. func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { return ctrl.NewControllerManagedBy(mgr). @@ -44,8 +36,7 @@ type Reconciler struct { //+kubebuilder:rbac:groups=toolchain.dev.openshift.com,resources=nstemplatetiers/status,verbs=get;update;patch //+kubebuilder:rbac:groups=toolchain.dev.openshift.com,resources=nstemplatetiers/finalizers,verbs=update -// Reconcile takes care of: -// - inserting a new entry in the `status.updates` +// Reconcile takes care of fetching the NSTemplateTier func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) @@ -66,44 +57,5 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, errs.Wrapf(err, "unable to get ToolchainConfig") } - // create a new entry in the `status.history` if needed - if added, err := r.ensureStatusUpdateRecord(ctx, tier); err != nil { - logger.Error(err, "unable to insert a new entry in status.updates after NSTemplateTier changed") - return reconcile.Result{}, errs.Wrap(err, "unable to insert a new entry in status.updates after NSTemplateTier changed") - } else if added { - logger.Info("Requeing after adding a new entry in tier.status.updates") - return reconcile.Result{Requeue: true}, nil - } return reconcile.Result{}, nil } - -// ensureStatusUpdateRecord adds a new entry in the `status.updates` with the current date/time -// if needed and the hash of the NSTemplateTier -// returns `true` if an entry was added, `err` if something wrong happened -func (r *Reconciler) ensureStatusUpdateRecord(ctx context.Context, tier *toolchainv1alpha1.NSTemplateTier) (bool, error) { - hash, err := hash.ComputeHashForNSTemplateTier(tier) - if err != nil { - return false, errs.Wrapf(err, "unable to append an entry in the `status.updates` for NSTemplateTier '%s'", tier.Name) - } - // if there was no previous status: - if len(tier.Status.Updates) == 0 { - return true, r.addNewTierUpdate(ctx, tier, hash) - } - - // check whether the entry was already added - logger := log.FromContext(ctx) - if tier.Status.Updates[len(tier.Status.Updates)-1].Hash == hash { - logger.Info("current tier template already exists in tier.status.updates") - return false, nil - } - logger.Info("Adding a new entry in tier.status.updates") - return true, r.addNewTierUpdate(ctx, tier, hash) -} - -func (r *Reconciler) addNewTierUpdate(ctx context.Context, tier *toolchainv1alpha1.NSTemplateTier, hash string) error { - tier.Status.Updates = append(tier.Status.Updates, toolchainv1alpha1.NSTemplateTierHistory{ - StartTime: metav1.Now(), - Hash: hash, - }) - return r.Client.Status().Update(ctx, tier) -} diff --git a/controllers/nstemplatetier/nstemplatetier_controller_test.go b/controllers/nstemplatetier/nstemplatetier_controller_test.go index 1125d2039..93369a559 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller_test.go +++ b/controllers/nstemplatetier/nstemplatetier_controller_test.go @@ -15,7 +15,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" @@ -34,77 +33,6 @@ func TestReconcile(t *testing.T) { // given logf.SetLogger(zap.New(zap.UseDevMode(true))) - t.Run("controller should add entry in tier.status.updates", func(t *testing.T) { - - t.Run("without previous entry", func(t *testing.T) { - // given - base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates) - initObjs := []runtimeclient.Object{base1nsTier} - r, req, cl := prepareReconcile(t, base1nsTier.Name, initObjs...) - // when - res, err := r.Reconcile(context.TODO(), req) - // then - require.NoError(t, err) - require.Equal(t, reconcile.Result{Requeue: true}, res) // explicit requeue after the adding an entry in `status.updates` - // check that an entry was added in `status.updates` - tiertest.AssertThatNSTemplateTier(t, "base1ns", cl). - HasStatusUpdatesItems(1). - HasLatestUpdate(toolchainv1alpha1.NSTemplateTierHistory{ - Hash: base1nsTier.Labels["toolchain.dev.openshift.com/base1ns-tier-hash"], - }) - }) - - t.Run("with previous entries", func(t *testing.T) { - // given - base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithPreviousUpdates( - toolchainv1alpha1.NSTemplateTierHistory{ - StartTime: metav1.Now(), - Hash: "abc123", - }, - toolchainv1alpha1.NSTemplateTierHistory{ - StartTime: metav1.Now(), - Hash: "def456", - })) - initObjs := []runtimeclient.Object{base1nsTier} - r, req, cl := prepareReconcile(t, base1nsTier.Name, initObjs...) - // when - res, err := r.Reconcile(context.TODO(), req) - // then - require.NoError(t, err) - require.Equal(t, reconcile.Result{Requeue: true}, res) // explicit requeue after the adding an entry in `status.updates` - // check that an entry was added in `status.updates` - tiertest.AssertThatNSTemplateTier(t, "base1ns", cl). - HasStatusUpdatesItems(3). - HasValidPreviousUpdates(). - HasLatestUpdate(toolchainv1alpha1.NSTemplateTierHistory{ - Hash: base1nsTier.Labels["toolchain.dev.openshift.com/base1ns-tier-hash"], - }) - }) - }) - - t.Run("controller should NOT add entry in tier.status.updates", func(t *testing.T) { - - t.Run("last entry exists with matching hash", func(t *testing.T) { - // given - base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, - tiertest.WithCurrentUpdate()) // current update already exists - - initObjs := []runtimeclient.Object{base1nsTier} - r, req, cl := prepareReconcile(t, base1nsTier.Name, initObjs...) - // when - res, err := r.Reconcile(context.TODO(), req) - // then - require.NoError(t, err) - require.Equal(t, reconcile.Result{}, res) // no explicit requeue - // check that no entry was added in `status.updates` - tiertest.AssertThatNSTemplateTier(t, "base1ns", cl). - HasStatusUpdatesItems(1). // same number of entries - HasLatestUpdate(toolchainv1alpha1.NSTemplateTierHistory{ - Hash: base1nsTier.Labels["toolchain.dev.openshift.com/base1ns-tier-hash"], - }) - }) - }) - t.Run("failures", func(t *testing.T) { t.Run("unable to get NSTemplateTier", func(t *testing.T) { @@ -147,27 +75,6 @@ func TestReconcile(t *testing.T) { }) }) - t.Run("unable to update NSTemplateTier status", func(t *testing.T) { - - t.Run("when adding new update", func(t *testing.T) { - // given - base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates) - initObjs := []runtimeclient.Object{base1nsTier} - r, req, cl := prepareReconcile(t, base1nsTier.Name, initObjs...) - cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.SubResourceUpdateOption) error { - if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { - return fmt.Errorf("mock error") - } - return cl.Client.Status().Update(ctx, obj, opts...) - } - // when - res, err := r.Reconcile(context.TODO(), req) - // then - require.EqualError(t, err, "unable to insert a new entry in status.updates after NSTemplateTier changed: mock error") - assert.Equal(t, reconcile.Result{}, res) // no explicit requeue - }) - }) - }) } diff --git a/test/nstemplatetier/assertion.go b/test/nstemplatetier/assertion.go deleted file mode 100644 index 1263b4632..000000000 --- a/test/nstemplatetier/assertion.go +++ /dev/null @@ -1,72 +0,0 @@ -package nstemplatetier - -import ( - "context" - - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/toolchain-common/pkg/test" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/types" - runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" -) - -// Assertion an assertion helper for an NSTemplateTier -type Assertion struct { - tier *toolchainv1alpha1.NSTemplateTier - client runtimeclient.Client - namespacedName types.NamespacedName - t test.T -} - -func (a *Assertion) loadResource() error { - tier := &toolchainv1alpha1.NSTemplateTier{} - err := a.client.Get(context.TODO(), a.namespacedName, tier) - a.tier = tier - return err -} - -// AssertThatNSTemplateTier helper func to begin with the assertions on an NSTemplateTier -func AssertThatNSTemplateTier(t test.T, name string, client runtimeclient.Client) *Assertion { - return &Assertion{ - client: client, - namespacedName: test.NamespacedName(test.HostOperatorNs, name), - t: t, - } -} - -// HasStatusUpdatesItems verifies the number of items in `status.updates` -func (a *Assertion) HasStatusUpdatesItems(expected int) *Assertion { - err := a.loadResource() - require.NoError(a.t, err) - require.Len(a.t, a.tier.Status.Updates, expected) - return a -} - -// HasValidPreviousUpdates verifies the previous `status.updates` -// in particular, it checks that: -// - `StartTime` is not nil -// - `Hash` is not nil -func (a *Assertion) HasValidPreviousUpdates() *Assertion { - err := a.loadResource() - require.NoError(a.t, err) - require.NotEmpty(a.t, a.tier.Status.Updates) - for _, h := range a.tier.Status.Updates[:len(a.tier.Status.Updates)-1] { - assert.NotNil(a.t, h.StartTime) - assert.NotNil(a.t, h.Hash) - } - return a -} - -// HasLatestUpdate verifies the latest `status.updates` -func (a *Assertion) HasLatestUpdate(expected toolchainv1alpha1.NSTemplateTierHistory) *Assertion { - err := a.loadResource() - require.NoError(a.t, err) - require.NotEmpty(a.t, a.tier.Status.Updates) - latest := a.tier.Status.Updates[len(a.tier.Status.Updates)-1] - assert.False(a.t, latest.StartTime.IsZero()) - assert.Equal(a.t, expected.Hash, latest.Hash) - - return a -} diff --git a/test/nstemplatetier/assertions.go b/test/nstemplatetier/assertions.go new file mode 100644 index 000000000..094784794 --- /dev/null +++ b/test/nstemplatetier/assertions.go @@ -0,0 +1,35 @@ +package nstemplatetier + +import ( + "context" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/toolchain-common/pkg/test" + + "k8s.io/apimachinery/pkg/types" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Assertion an assertion helper for an NSTemplateTier +type Assertion struct { + tier *toolchainv1alpha1.NSTemplateTier //nolint:golint,unused + client runtimeclient.Client + namespacedName types.NamespacedName + t test.T +} + +func (a *Assertion) loadResource() error { //nolint:golint,unused + tier := &toolchainv1alpha1.NSTemplateTier{} + err := a.client.Get(context.TODO(), a.namespacedName, tier) + a.tier = tier + return err +} + +// AssertThatNSTemplateTier helper func to begin with the assertions on an NSTemplateTier +func AssertThatNSTemplateTier(t test.T, name string, client runtimeclient.Client) *Assertion { + return &Assertion{ + client: client, + namespacedName: test.NamespacedName(test.HostOperatorNs, name), + t: t, + } +} diff --git a/test/nstemplatetier/nstemplatetier.go b/test/nstemplatetier/nstemplatetier.go index 3bdc80cae..931baabb1 100644 --- a/test/nstemplatetier/nstemplatetier.go +++ b/test/nstemplatetier/nstemplatetier.go @@ -178,29 +178,6 @@ func WithoutClusterResources() TierOption { } } -// WithPreviousUpdates adds the given entries in the `status.updates` -func WithPreviousUpdates(entries ...toolchainv1alpha1.NSTemplateTierHistory) TierOption { - return func(tier *toolchainv1alpha1.NSTemplateTier) { - tier.Status.Updates = entries - } -} - -// WithCurrentUpdate appends an entry in the `status.updates` for the current tier -func WithCurrentUpdate() TierOption { - return func(tier *toolchainv1alpha1.NSTemplateTier) { - hash, _ := hash.ComputeHashForNSTemplateTier(tier) - if tier.Status.Updates == nil { - tier.Status.Updates = []toolchainv1alpha1.NSTemplateTierHistory{} - } - tier.Status.Updates = append(tier.Status.Updates, - toolchainv1alpha1.NSTemplateTierHistory{ - StartTime: metav1.Now(), - Hash: hash, - }, - ) - } -} - // OtherTier returns an "other" NSTemplateTier func OtherTier() *toolchainv1alpha1.NSTemplateTier { return &toolchainv1alpha1.NSTemplateTier{