From f025813b80f16f126d2c18f5142b3cbf6da5f0b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20Schw=C3=A4gerl?= Date: Thu, 23 Jan 2025 12:48:53 +0100 Subject: [PATCH] chore: Add version and name helpers to ModuleTemplate (#2201) * chore: Add version and name helpers to ModuleTemplate * fix lint * fix lint ++ --- api/v1beta2/moduletemplate_types.go | 50 ++++--- api/v1beta2/moduletemplate_types_test.go | 131 ++++++++++++++++-- internal/descriptor/cache/key.go | 2 +- .../maintenancewindows/maintenance_window.go | 5 +- 4 files changed, 147 insertions(+), 41 deletions(-) diff --git a/api/v1beta2/moduletemplate_types.go b/api/v1beta2/moduletemplate_types.go index 2fc4b1360d..9c758ce316 100644 --- a/api/v1beta2/moduletemplate_types.go +++ b/api/v1beta2/moduletemplate_types.go @@ -223,39 +223,27 @@ func (m *ModuleTemplate) IsInternal() bool { return false } -var ErrInvalidVersion = errors.New("can't find valid semantic version") - -// getVersionLegacy() returns the version of the ModuleTemplate from the annotation on the object. -// Remove once shared.ModuleVersionAnnotation is removed. -func (m *ModuleTemplate) getVersionLegacy() (string, error) { - if m.Annotations != nil { - moduleVersion, found := m.Annotations[shared.ModuleVersionAnnotation] - if found { - return moduleVersion, nil - } +// https://github.com/kyma-project/lifecycle-manager/issues/2096 +// Refactor this function to drop the label fallback after the migration to the new ModuleTemplate format is completed. +func (m *ModuleTemplate) GetVersion() string { + version := m.Spec.Version + if version == "" { + version = m.Annotations[shared.ModuleVersionAnnotation] } - return "", ErrInvalidVersion + return version } -// GetVersion returns the declared version of the ModuleTemplate from it's Spec. -func (m *ModuleTemplate) GetVersion() (*semver.Version, error) { - var versionValue string - var err error - - if m.Spec.Version == "" { - versionValue, err = m.getVersionLegacy() - if err != nil { - return nil, err - } - } else { - versionValue = m.Spec.Version - } +var ErrInvalidVersion = errors.New("can't find valid semantic version") - version, err := semver.NewVersion(versionValue) +// GetSemanticVersion returns the declared version of the ModuleTemplate as semantic version. +func (m *ModuleTemplate) GetSemanticVersion() (*semver.Version, error) { + version := m.GetVersion() + + semanticVersion, err := semver.NewVersion(version) if err != nil { return nil, fmt.Errorf("%w: %s", ErrInvalidVersion, err.Error()) } - return version, nil + return semanticVersion, nil } // https://github.com/kyma-project/lifecycle-manager/issues/2096 @@ -279,3 +267,13 @@ func (m *ModuleTemplate) HasSyncDisabled() bool { } return false } + +// https://github.com/kyma-project/lifecycle-manager/issues/2096 +// Refactor this function to drop the label fallback after the migration to the new ModuleTemplate format is completed. +func (m *ModuleTemplate) GetModuleName() string { + moduleName := m.Spec.ModuleName + if moduleName == "" { + moduleName = m.Labels[shared.ModuleName] + } + return moduleName +} diff --git a/api/v1beta2/moduletemplate_types_test.go b/api/v1beta2/moduletemplate_types_test.go index b02b375e20..afaeafe7b9 100644 --- a/api/v1beta2/moduletemplate_types_test.go +++ b/api/v1beta2/moduletemplate_types_test.go @@ -4,13 +4,14 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" ) -func Test_GetVersion(t *testing.T) { +func Test_GetSemanticVersion(t *testing.T) { const testVersion = "1.0.1" const otherVersion = "0.0.1" tests := []struct { @@ -20,7 +21,7 @@ func Test_GetVersion(t *testing.T) { expectedErr string }{ { - name: "Test GetVersion() by annotation (legacy)", + name: "Test GetSemanticVersion() by annotation (legacy)", m: &v1beta2.ModuleTemplate{ ObjectMeta: apimetav1.ObjectMeta{ Annotations: map[string]string{ @@ -31,7 +32,7 @@ func Test_GetVersion(t *testing.T) { expectedVersion: testVersion, }, { - name: "Test GetVersion() by explicit version in Spec", + name: "Test GetSemanticVersion() by explicit version in Spec", m: &v1beta2.ModuleTemplate{ Spec: v1beta2.ModuleTemplateSpec{ Version: testVersion, @@ -40,7 +41,7 @@ func Test_GetVersion(t *testing.T) { expectedVersion: testVersion, }, { - name: "Test GetVersion() with both version in Spec and annotation", + name: "Test GetSemanticVersion() with both version in Spec and annotation", m: &v1beta2.ModuleTemplate{ ObjectMeta: apimetav1.ObjectMeta{ Annotations: map[string]string{ @@ -75,32 +76,140 @@ func Test_GetVersion(t *testing.T) { } for _, testCase := range tests { t.Run(testCase.name, func(t *testing.T) { - actualVersion, err := testCase.m.GetVersion() + actualVersion, err := testCase.m.GetSemanticVersion() if err != nil { if actualVersion != nil { - t.Errorf("GetVersion(): Returned version should be nil when error is not nil") + t.Errorf("GetSemanticVersion(): Returned version should be nil when error is not nil") } if testCase.expectedErr == "" { - t.Errorf("GetVersion(): Unexpected error: %v", err) + t.Errorf("GetSemanticVersion(): Unexpected error: %v", err) } if !strings.Contains(err.Error(), testCase.expectedErr) { - t.Errorf("GetVersion(): Actual error = %v, expected error: %v", err, testCase.expectedErr) + t.Errorf("GetSemanticVersion(): Actual error = %v, expected error: %v", err, testCase.expectedErr) } return } if actualVersion == nil { - t.Errorf("GetVersion(): Returned version should not be nil when error is nil") + t.Errorf("GetSemanticVersion(): Returned version should not be nil when error is nil") } if testCase.expectedVersion == "" { - t.Errorf("GetVersion(): Expected version is empty but non-nil version is returned") + t.Errorf("GetSemanticVersion(): Expected version is empty but non-nil version is returned") } if actualVersion != nil && actualVersion.String() != testCase.expectedVersion { - t.Errorf("GetVersion(): actual version = %v, expected version: %v", actualVersion.String(), + t.Errorf("GetSemanticVersion(): actual version = %v, expected version: %v", actualVersion.String(), testCase.expectedVersion) } }) } } + +//nolint:dupl // similar but not duplicate +func Test_GetVersion(t *testing.T) { + tests := []struct { + name string + m *v1beta2.ModuleTemplate + expectedVersion string + }{ + { + name: "Test GetVersion() by annotation (legacy)", + m: &v1beta2.ModuleTemplate{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + shared.ModuleVersionAnnotation: "1.0.0-annotated", + }, + }, + Spec: v1beta2.ModuleTemplateSpec{}, + }, + expectedVersion: "1.0.0-annotated", + }, + { + name: "Test GetVersion() by spec.version", + m: &v1beta2.ModuleTemplate{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + Spec: v1beta2.ModuleTemplateSpec{ + Version: "2.0.0-spec", + }, + }, + expectedVersion: "2.0.0-spec", + }, + { + name: "Test GetVersion() spec.moduleName has priority over annotation", + m: &v1beta2.ModuleTemplate{ + ObjectMeta: apimetav1.ObjectMeta{ + Annotations: map[string]string{ + shared.ModuleVersionAnnotation: "1.0.0-annotated", + }, + }, + Spec: v1beta2.ModuleTemplateSpec{ + Version: "2.0.0-spec", + }, + }, + expectedVersion: "2.0.0-spec", + }, + } + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + actualVersion := testCase.m.GetVersion() + assert.Equal(t, testCase.expectedVersion, actualVersion) + }) + } +} + +//nolint:dupl // similar but not duplicate +func Test_GetModuleName(t *testing.T) { + tests := []struct { + name string + m *v1beta2.ModuleTemplate + expectedName string + }{ + { + name: "Test GetModuleName() by label", + m: &v1beta2.ModuleTemplate{ + ObjectMeta: apimetav1.ObjectMeta{ + Labels: map[string]string{ + shared.ModuleName: "labelled-module", + }, + }, + Spec: v1beta2.ModuleTemplateSpec{}, + }, + expectedName: "labelled-module", + }, + { + name: "Test GetModuleName() by spec.moduleName", + m: &v1beta2.ModuleTemplate{ + ObjectMeta: apimetav1.ObjectMeta{ + Labels: map[string]string{}, + }, + Spec: v1beta2.ModuleTemplateSpec{ + ModuleName: "spec-module", + }, + }, + expectedName: "spec-module", + }, + { + name: "Test GetModuleName() spec.moduleName has priority over label", + m: &v1beta2.ModuleTemplate{ + ObjectMeta: apimetav1.ObjectMeta{ + Labels: map[string]string{ + shared.ModuleName: "labelled-module", + }, + }, + Spec: v1beta2.ModuleTemplateSpec{ + ModuleName: "spec-module", + }, + }, + expectedName: "spec-module", + }, + } + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + actualName := testCase.m.GetModuleName() + assert.Equal(t, testCase.expectedName, actualName) + }) + } +} diff --git a/internal/descriptor/cache/key.go b/internal/descriptor/cache/key.go index f5d7cf90e2..4a6472f663 100644 --- a/internal/descriptor/cache/key.go +++ b/internal/descriptor/cache/key.go @@ -9,7 +9,7 @@ import ( type DescriptorKey string func GenerateDescriptorKey(template *v1beta2.ModuleTemplate) DescriptorKey { - version, err := template.GetVersion() + version, err := template.GetSemanticVersion() if err == nil { return DescriptorKey(fmt.Sprintf("%s:%s:%d:%s", template.Name, template.Spec.Channel, template.Generation, version)) diff --git a/internal/maintenancewindows/maintenance_window.go b/internal/maintenancewindows/maintenance_window.go index 2f1a5389f4..ce358ebdce 100644 --- a/internal/maintenancewindows/maintenance_window.go +++ b/internal/maintenancewindows/maintenance_window.go @@ -73,14 +73,13 @@ func (MaintenanceWindow) IsRequired(moduleTemplate *v1beta2.ModuleTemplate, kyma } // module not installed yet => no need for maintenance window - moduleStatus := kyma.Status.GetModuleStatus(moduleTemplate.Spec.ModuleName) + moduleStatus := kyma.Status.GetModuleStatus(moduleTemplate.GetModuleName()) if moduleStatus == nil { return false } // module already installed in this version => no need for maintenance window - installedVersion := moduleStatus.Version - return installedVersion != moduleTemplate.Spec.Version + return moduleStatus.Version != moduleTemplate.GetVersion() } // IsActive determines if a maintenance window is currently active.