Skip to content

Simplify nstemplatetiers.GenerateTiers #486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
metlos merged 2 commits into from
Jul 8, 2025
+35 −77
Merged

Simplify nstemplatetiers.GenerateTiers #486

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf062f9
simplify nstemplatetiers.GenerateTiers to not distiniguish between cr…
metlos Jun 26, 2025
3c1e49a
be clever about detecting the generation change and try to detect pos…
metlos Jul 3, 2025
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
17 changes: 4 additions & 13 deletions pkg/template/nstemplatetiers/nstemplatetier_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (

var log = logf.Log.WithName("templates")

type EnsureObject func(toEnsure runtimeclient.Object, canUpdate bool, tierName string) (bool, error)
type EnsureObject func(toEnsure runtimeclient.Object, tierName string) error

type TierGenerator struct {
ensureObject EnsureObject
Expand Down Expand Up @@ -76,7 +76,6 @@ func GenerateTiers(s *runtime.Scheme, ensureObject EnsureObject, namespace strin

// newNSTemplateTierGenerator loads templates from the provided assets and processes the tierTemplates and NSTemplateTiers
func newNSTemplateTierGenerator(s *runtime.Scheme, ensureObject EnsureObject, namespace string, metadata map[string]string, files map[string][]byte) (*TierGenerator, error) {

templatesByTier, err := loadTemplatesByTiers(metadata, files)
if err != nil {
return nil, err
Expand Down Expand Up @@ -141,7 +140,6 @@ type BasedOnTier struct {
// an optional `template` for the cluster resources (`clusterTemplate`) and the NSTemplateTier resource object.
// Each `template` object contains a `revision` (`string`) and the `content` of the template to apply (`[]byte`)
func loadTemplatesByTiers(metadata map[string]string, files map[string][]byte) (map[string]*tierData, error) {

results := make(map[string]*tierData)
for name, content := range files {
// split the name using the `/` separator
Expand Down Expand Up @@ -203,7 +201,6 @@ func loadTemplatesByTiers(metadata map[string]string, files map[string][]byte) (

// initTierTemplates generates all TierTemplate resources, and adds them to the tier map indexed by tier name
func (t *TierGenerator) initTierTemplates() error {

// process tiers in alphabetical order
tiers := make([]string, 0, len(t.templatesByTier))
for tier := range t.templatesByTier {
Expand Down Expand Up @@ -279,7 +276,7 @@ func (t *TierGenerator) createTierTemplates() error {
for _, tierTmpl := range tierTmpls.tierTemplates {
log.Info("creating TierTemplate", "namespace", tierTmpl.Namespace, "name", tierTmpl.Name)
// using the "standard" client since we don't need to support updates on such resources, they should be immutable
if _, err := t.ensureObject(tierTmpl, false, tierName); err != nil {
if err := t.ensureObject(tierTmpl, tierName); err != nil {
return errors.Wrapf(err, "unable to create the '%s' TierTemplate in namespace '%s'", tierTmpl.Name, tierTmpl.Namespace)
}
log.Info("TierTemplate resource created", "namespace", tierTmpl.Namespace, "name", tierTmpl.Name)
Expand Down Expand Up @@ -336,7 +333,6 @@ func newTierTemplateName(tier, kind, revision string) string {

// newNSTemplateTiers generates all NSTemplateTier resources and adds them to the tier map
func (t *TierGenerator) initNSTemplateTiers() error {

for tierName, tierData := range t.templatesByTier {
nsTemplateTier := tierData.rawTemplates.nsTemplateTier
tierTemplates := tierData.tierTemplates
Expand All @@ -360,7 +356,6 @@ func (t *TierGenerator) initNSTemplateTiers() error {

// createNSTemplateTiers creates the NSTemplateTier resources from the tier map
func (t *TierGenerator) createNSTemplateTiers() error {

for tierName, tierData := range t.templatesByTier {
if len(tierData.objects) != 1 {
return fmt.Errorf("there is an unexpected number of NSTemplateTier object to be applied for tier name '%s'; expected: 1; actual: %d", tierName, len(tierData.objects))
Expand All @@ -380,7 +375,7 @@ func (t *TierGenerator) createNSTemplateTiers() error {
labels = make(map[string]string)
}
labels[toolchainv1alpha1.ProviderLabelKey] = toolchainv1alpha1.ProviderLabelValue
updated, err := t.ensureObject(tier, true, tierName)
err := t.ensureObject(tier, tierName)
if err != nil {
return errors.Wrapf(err, "unable to create or update the '%s' NSTemplateTier", tierName)
}
Expand All @@ -394,11 +389,7 @@ func (t *TierGenerator) createNSTemplateTiers() error {
for role, nsTemplate := range tier.Spec.SpaceRoles {
tierLog = tierLog.WithValues(fmt.Sprintf("spaceRoleTemplate-%s", role), nsTemplate.TemplateRef)
}
if updated {
tierLog.Info("NSTemplateTier was either updated or created")
} else {
tierLog.Info("NSTemplateTier wasn't updated nor created: the spec was already set as expected")
}
tierLog.Info("NSTemplateTier was patched")
}
return nil
}
Expand Down
81 changes: 24 additions & 57 deletions pkg/template/nstemplatetiers/nstemplatetier_generator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"io/fs"
"path/filepath"
"reflect"
"regexp"
"testing"
texttemplate "text/template"
Expand All @@ -17,8 +18,6 @@ import (
commonclient "github.com/codeready-toolchain/toolchain-common/pkg/client"
"github.com/codeready-toolchain/toolchain-common/pkg/test"
"github.com/pkg/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"

Expand Down Expand Up @@ -141,12 +140,10 @@ func getTestTemplates(t *testing.T) map[string][]byte {
}

func TestGenerateTiers(t *testing.T) {

s := addToScheme(t)
logf.SetLogger(zap.New(zap.UseDevMode(true)))

t.Run("ok", func(t *testing.T) {

expectedTemplateRefs := map[string]map[string]interface{}{
"advanced": {
"clusterresources": "advanced-clusterresources-abcd123-654321a",
Expand Down Expand Up @@ -280,19 +277,29 @@ func TestGenerateTiers(t *testing.T) {
// when
err := GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t))
require.NoError(t, err)
// here we're just capturing the state after the first call so that we can compare with the state after the second call
tierTmpls := toolchainv1alpha1.TierTemplateList{}
err = clt.List(context.TODO(), &tierTmpls, runtimeclient.InNamespace(namespace))
require.NoError(t, err)
require.Len(t, tierTmpls.Items, 16) // 4 items for advanced and base tiers + 3 for nocluster tier + 4 for appstudio
origTemplates := map[string]*toolchainv1alpha1.TierTemplate{}
for _, tmpl := range tierTmpls.Items {
origTemplates[tmpl.Name] = &tmpl
}

// when calling CreateOrUpdateResources a second time
err = GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t))

// then
require.NoError(t, err)
// verify that all TierTemplate CRs were updated
tierTmpls := toolchainv1alpha1.TierTemplateList{}
tierTmpls = toolchainv1alpha1.TierTemplateList{}
err = clt.List(context.TODO(), &tierTmpls, runtimeclient.InNamespace(namespace))
require.NoError(t, err)
require.Len(t, tierTmpls.Items, 16) // 4 items for advanced and base tiers + 3 for nocluster tier + 4 for appstudio
// check that the tier templates are unchanged
for _, tierTmpl := range tierTmpls.Items {
assert.Equal(t, int64(1), tierTmpl.Generation) // unchanged
assert.True(t, reflect.DeepEqual(origTemplates[tierTmpl.Name].Spec, tierTmpl.Spec))
Copy link
Collaborator

@alexeykazakov alexeykazakov Jul 3, 2025

Choose a reason for hiding this comment

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

You could do

Suggested change
assert.True(t, reflect.DeepEqual(origTemplates[tierTmpl.Name].Spec, tierTmpl.Spec))
assert.Equal(t, origTemplates[tierTmpl.Name].Spec, tierTmpl.Spec)

instead, right?
Or why would the generation change here? The tier is not going to be updated for the same template, right? Basically why you need to change the test here at all?

Copy link
Contributor Author

@metlos metlos Jul 3, 2025

Choose a reason for hiding this comment

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

Huh, this led me down a rabbit hole. I changed this because the generation was being changed during a single patch call, so I was suspecting our test client doing something weird.

And it indeed was weird. The actual "error" because of which the generation was changed to 2 was caused by our implementation of test.Patch (the default patching behavior on top of the fake client).

We want determine whether to increase the generation there by reading the object from the cluster, dry-running the apply on a copy of the object and then comparing the two.

Now, the "original" object comes directly from the filesystem and contains the template as "raw extension", that is read as just a []byte. When you save it to the cluster, the raw extension gets serialized for some reason.

Now, because some of the templates explicitly declared a zero value of some properties (namely ClusterResourceQuota.spec.selector.annotation) the representation didn't match what came deserialized back from the cluster (which didn't contain the zero value anymore, understandably).

To capture this in the future, I added a comment and a second check on the same place - one check is using my method of deep equal and the other is the original generation change check. If we see just one of them failing, we know where to look the next time.

Thanks for pointing this out. Took me a bit to figure this out but the test is now better.

Copy link
Collaborator

@alexeykazakov alexeykazakov Jul 3, 2025

Choose a reason for hiding this comment

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

Nice investigation!

}

// verify that 4 NSTemplateTier CRs were created:
Expand Down Expand Up @@ -446,27 +453,25 @@ func TestGenerateTiers(t *testing.T) {
})

t.Run("failures", func(t *testing.T) {

namespace := "host-operator" + uuid.NewString()[:7]

t.Run("nstemplatetiers", func(t *testing.T) {

t.Run("failed to create nstemplatetiers", func(t *testing.T) {
t.Run("failed to patch nstemplatetiers", func(t *testing.T) {
// given
clt := test.NewFakeClient(t)
clt.MockCreate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.CreateOption) error {
if obj.GetObjectKind().GroupVersionKind().Kind == "NSTemplateTier" {
clt.MockPatch = func(ctx context.Context, obj runtimeclient.Object, patch runtimeclient.Patch, opts ...runtimeclient.PatchOption) error {
if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok {
// simulate a client/server error
return errors.Errorf("an error")
}
return clt.Client.Create(ctx, obj, opts...)
return test.Patch(ctx, clt, obj, patch, opts...)
}

// when
err := GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t))
// then
require.Error(t, err)
assert.Regexp(t, "unable to create or update the '\\w+' NSTemplateTier: unable to create resource of kind: NSTemplateTier, version: v1alpha1: an error", err.Error())
assert.Regexp(t, "unable to create NSTemplateTiers: unable to create or update the '\\w+' NSTemplateTier: unable to patch 'toolchain.dev.openshift.com/v1alpha1, Kind=NSTemplateTier' called '\\w+' in namespace '[a-zA-Z0-9-]+': an error", err.Error())
})

t.Run("missing tier.yaml file", func(t *testing.T) {
Expand All @@ -481,40 +486,15 @@ func TestGenerateTiers(t *testing.T) {
require.EqualError(t, err, "unable to init NSTemplateTier generator: tier appstudio is missing a tier.yaml file")
})

t.Run("failed to update nstemplatetiers", func(t *testing.T) {
// given
// initialize the client with an existing `advanced` NSTemplatetier
clt := test.NewFakeClient(t, &toolchainv1alpha1.NSTemplateTier{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: "advanced",
},
})
clt.MockUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error {
if obj.GetObjectKind().GroupVersionKind().Kind == "NSTemplateTier" {
// simulate a client/server error
return errors.Errorf("an error")
}
return clt.Client.Update(ctx, obj, opts...)
}

// when
err := GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t))

// then
require.Error(t, err)
assert.Contains(t, err.Error(), "unable to create NSTemplateTiers: unable to create or update the 'advanced' NSTemplateTier: unable to create resource of kind: NSTemplateTier, version: v1alpha1: unable to update the resource")
})

t.Run("failed to create nstemplatetiers", func(t *testing.T) {
t.Run("failed to patch nstemplatetiers", func(t *testing.T) {
// given
clt := test.NewFakeClient(t)
clt.MockCreate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.CreateOption) error {
clt.MockPatch = func(ctx context.Context, obj runtimeclient.Object, patch runtimeclient.Patch, opts ...runtimeclient.PatchOption) error {
if _, ok := obj.(*toolchainv1alpha1.TierTemplate); ok {
// simulate a client/server error
return errors.Errorf("an error")
}
return clt.Client.Create(ctx, obj, opts...)
return test.Patch(ctx, clt, obj, patch, opts...)
}

// when
Expand All @@ -529,7 +509,6 @@ func TestGenerateTiers(t *testing.T) {
}

func TestLoadTemplatesByTiers(t *testing.T) {

logf.SetLogger(zap.New(zap.UseDevMode(true)))

t.Run("ok", func(t *testing.T) {
Expand Down Expand Up @@ -591,7 +570,6 @@ func TestLoadTemplatesByTiers(t *testing.T) {
})

t.Run("failures", func(t *testing.T) {

t.Run("unparseable content", func(t *testing.T) {
// given
testTemplates := getTestTemplates(t)
Expand Down Expand Up @@ -663,13 +641,11 @@ func TestLoadTemplatesByTiers(t *testing.T) {
}

func TestNewNSTemplateTier(t *testing.T) {

s := scheme.Scheme
err := toolchainv1alpha1.AddToScheme(s)
require.NoError(t, err)

t.Run("ok", func(t *testing.T) {

t.Run("with test assets", func(t *testing.T) {
// given
namespace := "host-operator-" + uuid.NewString()[:7]
Expand Down Expand Up @@ -740,7 +716,6 @@ func TestNewTierTemplate(t *testing.T) {
namespace := "host-operator-" + uuid.NewString()[:7]

t.Run("ok", func(t *testing.T) {

t.Run("with test assets", func(t *testing.T) {
// given

Expand Down Expand Up @@ -780,7 +755,6 @@ func TestNewTierTemplate(t *testing.T) {
})

t.Run("failures", func(t *testing.T) {

t.Run("invalid template", func(t *testing.T) {
// given
testTemplates := getTestTemplates(t)
Expand All @@ -796,15 +770,9 @@ func TestNewTierTemplate(t *testing.T) {
}

func ensureObjectFuncForClient(cl runtimeclient.Client) EnsureObject {
return func(toEnsure runtimeclient.Object, canUpdate bool, _ string) (bool, error) {
if !canUpdate {
if err := cl.Create(context.TODO(), toEnsure); err != nil && !apierrors.IsAlreadyExists(err) {
return false, err
}
return true, nil
}
applyCl := commonclient.NewApplyClient(cl)
return applyCl.ApplyObject(context.TODO(), toEnsure, commonclient.ForceUpdate(true))
return func(toEnsure runtimeclient.Object, _ string) error {
applyCl := commonclient.NewSSAApplyClient(cl, "testFieldManager")
return applyCl.ApplyObject(context.TODO(), toEnsure)
}
}

Expand Down Expand Up @@ -862,7 +830,6 @@ func expectedTemplateFromBasedOnTierConfig(t *testing.T, tier, templateFileName
}

func TestNewNSTemplateTiers(t *testing.T) {

// given
s := addToScheme(t)

Expand Down
Loading