From e85adcaa30add65f8f936b1bee4f5d9d0dacbfc0 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Sun, 10 Mar 2024 22:39:31 +0100 Subject: [PATCH] Add test cases to get helmcharts to use chartRefs Signed-off-by: Soule BA --- api/v2beta2/helmrelease_types.go | 12 +- api/v2beta2/reference_types.go | 4 +- api/v2beta2/zz_generated.deepcopy.go | 10 +- .../helm.toolkit.fluxcd.io_helmreleases.yaml | 3 +- docs/api/v2beta2/helm.md | 18 +-- internal/controller/helmrelease_controller.go | 18 +-- .../controller/helmrelease_controller_test.go | 105 ++++++++++++++++++ internal/reconcile/helmchart_template.go | 30 ++--- 8 files changed, 151 insertions(+), 49 deletions(-) diff --git a/api/v2beta2/helmrelease_types.go b/api/v2beta2/helmrelease_types.go index a81b1ddb8..391b5daac 100644 --- a/api/v2beta2/helmrelease_types.go +++ b/api/v2beta2/helmrelease_types.go @@ -84,7 +84,7 @@ type HelmReleaseSpec struct { // ChartRef holds a reference to a source controller resource containing the // Helm chart artifact. // +optional - ChartRef *CrosssNameSpaceSourceReference `json:"chartRef,omitempty"` + ChartRef *CrossNamespaceSourceReference `json:"chartRef,omitempty"` // Interval at which to reconcile the Helm release. // +kubebuilder:validation:Type=string @@ -1247,6 +1247,16 @@ func (in *HelmRelease) GetStatusConditions() *[]metav1.Condition { return &in.Status.Conditions } +// IsChartRefPresent returns true if the HelmRelease has a ChartRef. +func (in *HelmRelease) IsChartRefPresent() bool { + return in.Spec.ChartRef != nil +} + +// IsChartTemplatePresent returns true if the HelmRelease has a ChartTemplate. +func (in *HelmRelease) IsChartTemplatePresent() bool { + return in.Spec.Chart.Spec.Chart != "" +} + // +kubebuilder:object:root=true // HelmReleaseList contains a list of HelmRelease objects. diff --git a/api/v2beta2/reference_types.go b/api/v2beta2/reference_types.go index 40acd5c68..43b722703 100644 --- a/api/v2beta2/reference_types.go +++ b/api/v2beta2/reference_types.go @@ -42,9 +42,9 @@ type CrossNamespaceObjectReference struct { Namespace string `json:"namespace,omitempty"` } -// CrosssNameSpaceSourceReference contains enough information to let you locate +// CrossNamespaceSourceReference contains enough information to let you locate // the typed referenced object at cluster level. -type CrosssNameSpaceSourceReference struct { +type CrossNamespaceSourceReference struct { // APIVersion of the referent. // +optional APIVersion string `json:"apiVersion,omitempty"` diff --git a/api/v2beta2/zz_generated.deepcopy.go b/api/v2beta2/zz_generated.deepcopy.go index 0d4f6491f..fa91287ed 100644 --- a/api/v2beta2/zz_generated.deepcopy.go +++ b/api/v2beta2/zz_generated.deepcopy.go @@ -45,16 +45,16 @@ func (in *CrossNamespaceObjectReference) DeepCopy() *CrossNamespaceObjectReferen } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *CrosssNameSpaceSourceReference) DeepCopyInto(out *CrosssNameSpaceSourceReference) { +func (in *CrossNamespaceSourceReference) DeepCopyInto(out *CrossNamespaceSourceReference) { *out = *in } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CrosssNameSpaceSourceReference. -func (in *CrosssNameSpaceSourceReference) DeepCopy() *CrosssNameSpaceSourceReference { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CrossNamespaceSourceReference. +func (in *CrossNamespaceSourceReference) DeepCopy() *CrossNamespaceSourceReference { if in == nil { return nil } - out := new(CrosssNameSpaceSourceReference) + out := new(CrossNamespaceSourceReference) in.DeepCopyInto(out) return out } @@ -262,7 +262,7 @@ func (in *HelmReleaseSpec) DeepCopyInto(out *HelmReleaseSpec) { in.Chart.DeepCopyInto(&out.Chart) if in.ChartRef != nil { in, out := &in.ChartRef, &out.ChartRef - *out = new(CrosssNameSpaceSourceReference) + *out = new(CrossNamespaceSourceReference) **out = **in } out.Interval = in.Interval diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index f62730f0b..0deedb57e 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -1293,7 +1293,8 @@ spec: - spec type: object chartRef: - description: ChartRef holds a reference to a v1beta2.HelmChart resource + description: ChartRef holds a reference to a source controller resource + containing the Helm chart artifact. properties: apiVersion: description: APIVersion of the referent. diff --git a/docs/api/v2beta2/helm.md b/docs/api/v2beta2/helm.md index 3cdf75032..9473052d0 100644 --- a/docs/api/v2beta2/helm.md +++ b/docs/api/v2beta2/helm.md @@ -87,14 +87,15 @@ for this HelmRelease.

chartRef
- -CrosssNameSpaceSourceReference + +CrossNamespaceSourceReference (Optional) -

ChartRef holds a reference to a v1beta2.HelmChart resource

+

ChartRef holds a reference to a source controller resource containing the +Helm chart artifact.

@@ -484,13 +485,13 @@ string -

CrosssNameSpaceSourceReference +

CrossNamespaceSourceReference

(Appears on: HelmReleaseSpec)

-

CrosssNameSpaceSourceReference contains enough information to let you locate +

CrossNamespaceSourceReference contains enough information to let you locate the typed referenced object at cluster level.

@@ -1097,14 +1098,15 @@ for this HelmRelease.

chartRef
- -CrosssNameSpaceSourceReference + +CrossNamespaceSourceReference (Optional) -

ChartRef holds a reference to a v1beta2.HelmChart resource

+

ChartRef holds a reference to a source controller resource containing the +Helm chart artifact.

diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 85f47f9c3..be3ac103f 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -673,7 +673,7 @@ func (r *HelmReleaseReconciler) buildRESTClientGetter(ctx context.Context, obj * // It returns the source object or an error. func (r *HelmReleaseReconciler) getSource(ctx context.Context, obj *v2.HelmRelease) (source.Source, error) { var name, namespace string - if isChartRefPresent(obj) { + if obj.IsChartRefPresent() { if obj.Spec.ChartRef.Kind == sourcev1.OCIRepositoryKind { return r.getHelmChartFromOCIRef(ctx, obj) } @@ -855,26 +855,18 @@ func isOCIRepositoryReady(obj *sourcev1.OCIRepository) (bool, string) { } } -func isChartRefPresent(obj *v2.HelmRelease) bool { - return obj.Spec.ChartRef != nil -} - -func isChartTemplatePresent(obj *v2.HelmRelease) bool { - return obj.Spec.Chart.Spec.Chart != "" -} - func isValidChartRef(obj *v2.HelmRelease) bool { - return (isChartRefPresent(obj) && !isChartTemplatePresent(obj)) || - (!isChartRefPresent(obj) && isChartTemplatePresent(obj)) + return (obj.IsChartRefPresent() && !obj.IsChartTemplatePresent()) || + (!obj.IsChartRefPresent() && obj.IsChartTemplatePresent()) } func getNamespacedName(obj *v2.HelmRelease) (types.NamespacedName, error) { namespacedName := types.NamespacedName{} switch { - case isChartRefPresent(obj) && !isChartTemplatePresent(obj): + case obj.IsChartRefPresent() && !obj.IsChartTemplatePresent(): namespacedName.Namespace = obj.Spec.ChartRef.Namespace namespacedName.Name = obj.Spec.ChartRef.Name - case !isChartRefPresent(obj) && isChartTemplatePresent(obj): + case !obj.IsChartRefPresent() && obj.IsChartTemplatePresent(): namespacedName.Namespace = obj.Spec.Chart.GetNamespace(obj.GetNamespace()) namespacedName.Name = obj.GetHelmChartName() default: diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index 0c8138d79..a2179739d 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -2016,6 +2016,25 @@ func TestHelmReleaseReconciler_getHelmChart(t *testing.T) { wantErr: true, disallowCrossNS: true, }, + { + "get chart from reference", + &v2.HelmRelease{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + Spec: v2.HelmReleaseSpec{ + ChartRef: &v2.CrossNamespaceSourceReference{ + Kind: "HelmChart", + Name: "some-chart-name", + Namespace: "some-namespace", + }, + }, + }, + chart, + true, + false, + false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2414,3 +2433,89 @@ func Test_isHelmChartReady(t *testing.T) { }) } } + +func Test_isOCIRepositoryReady(t *testing.T) { + mock := &sourcev1b2.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mock", + Namespace: "default", + Generation: 2, + }, + Status: sourcev1b2.OCIRepositoryStatus{ + ObservedGeneration: 2, + Conditions: []metav1.Condition{ + { + Type: meta.ReadyCondition, + Status: metav1.ConditionTrue, + }, + }, + Artifact: &sourcev1.Artifact{}, + }, + } + + tests := []struct { + name string + obj *sourcev1b2.OCIRepository + want bool + wantReason string + }{ + { + name: "OCIRepository is ready", + obj: mock.DeepCopy(), + want: true, + }, + { + name: "OCIRepository generation differs from observed generation while Ready=True", + obj: func() *sourcev1b2.OCIRepository { + m := mock.DeepCopy() + m.Generation = 3 + return m + }(), + want: false, + wantReason: "OCIRepository 'default/mock' is not ready: latest generation of object has not been reconciled", + }, + { + name: "OCIRepository generation differs from observed generation while Ready=False", + obj: func() *sourcev1b2.OCIRepository { + m := mock.DeepCopy() + m.Generation = 3 + conditions.MarkFalse(m, meta.ReadyCondition, "Reason", "some reason") + return m + }(), + want: false, + wantReason: "OCIRepository 'default/mock' is not ready: some reason", + }, + { + name: "OCIRepository has Stalled=True", + obj: func() *sourcev1b2.OCIRepository { + m := mock.DeepCopy() + conditions.MarkFalse(m, meta.ReadyCondition, "Reason", "some reason") + conditions.MarkStalled(m, "Reason", "some stalled reason") + return m + }(), + want: false, + wantReason: "OCIRepository 'default/mock' is not ready: some stalled reason", + }, + { + name: "OCIRepository does not have an Artifact", + obj: func() *sourcev1b2.OCIRepository { + m := mock.DeepCopy() + m.Status.Artifact = nil + return m + }(), + want: false, + wantReason: "OCIRepository 'default/mock' is not ready: does not have an artifact", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, gotReason := isOCIRepositoryReady(tt.obj) + if got != tt.want { + t.Errorf("isOCIRepositoryReady() got = %v, want %v", got, tt.want) + } + if gotReason != tt.wantReason { + t.Errorf("isOCIRepositoryReady() reason = %v, want %v", gotReason, tt.wantReason) + } + }) + } +} diff --git a/internal/reconcile/helmchart_template.go b/internal/reconcile/helmchart_template.go index c7d9d77b6..57fd257fd 100644 --- a/internal/reconcile/helmchart_template.go +++ b/internal/reconcile/helmchart_template.go @@ -76,24 +76,6 @@ func NewHelmChartTemplate(client client.Client, recorder record.EventRecorder, f } } -func isChartRefPresent(obj *v2.HelmRelease) bool { - return obj.Spec.ChartRef != nil -} - -func isChartTemplatePresent(obj *v2.HelmRelease) bool { - return obj.Spec.Chart.Spec.Chart != "" -} - -func mustCleanDeployedChart(obj *v2.HelmRelease) bool { - if isChartRefPresent(obj) && !isChartTemplatePresent(obj) { - if obj.Status.HelmChart != "" { - return true - } - } - - return false -} - func (r *HelmChartTemplate) Reconcile(ctx context.Context, req *Request) error { var ( obj = req.Object @@ -122,7 +104,7 @@ func (r *HelmChartTemplate) Reconcile(ctx context.Context, req *Request) error { return nil } - if isChartRefPresent(obj) { + if obj.IsChartRefPresent() { // if a chartRef is present, we do not need to reconcile the HelmChart from the template. return nil } @@ -262,3 +244,13 @@ func buildHelmChartFromTemplate(obj *v2.HelmRelease) *sourcev1.HelmChart { } return result } + +func mustCleanDeployedChart(obj *v2.HelmRelease) bool { + if obj.IsChartRefPresent() && !obj.IsChartTemplatePresent() { + if obj.Status.HelmChart != "" { + return true + } + } + + return false +}