Skip to content

Commit 46d2ca8

Browse files
roshprcursoragent
roshpr
and
cursoragent
committed
Require NoIndex for OCI static Ready
Stale Ready=True with reason Succeeded (or empty) was treated as a completed OCI migration, so the one-shot reconcile that sets NoIndex could be skipped. Require Ready=True with reason NoIndex before skipping migration, and assert that in predicate and reconciler tests. Assisted-by: Cursor Grok 4.6/cursor-grok-4.6-high-fast Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Rosh Ramadass <roshpr@gmail.com>
1 parent e38869c commit 46d2ca8

3 files changed

Lines changed: 133 additions & 15 deletions

File tree

‎internal/controller/helmrepository_controller_test.go‎

Lines changed: 84 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1752,7 +1752,10 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
17521752
if err := testEnv.Get(ctx, key, obj); err != nil {
17531753
return false
17541754
}
1755+
ready := conditions.Get(obj, meta.ReadyCondition)
17551756
return newGen == obj.Generation &&
1757+
conditions.IsReady(obj) &&
1758+
ready != nil && ready.Reason == sourcev1.NoIndexReason &&
17561759
!intpredicates.HelmRepositoryOCIRequireMigration(obj)
17571760
}, timeout).Should(BeTrue())
17581761

@@ -1965,10 +1968,11 @@ func TestHelmRepositoryReconciler_ociMigration(t *testing.T) {
19651968

19661969
g.Eventually(func() bool {
19671970
_ = testEnv.Get(ctx, hrKey, hr)
1968-
return !intpredicates.HelmRepositoryOCIRequireMigration(hr)
1971+
ready := conditions.Get(hr, meta.ReadyCondition)
1972+
return conditions.IsReady(hr) &&
1973+
ready != nil && ready.Reason == sourcev1.NoIndexReason &&
1974+
!intpredicates.HelmRepositoryOCIRequireMigration(hr)
19691975
}, timeout, time.Second).Should(BeTrue())
1970-
g.Expect(conditions.IsReady(hr)).To(BeTrue())
1971-
g.Expect(conditions.Get(hr, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
19721976
g.Expect(controllerutil.ContainsFinalizer(hr, sourcev1.SourceFinalizer)).To(BeFalse())
19731977

19741978
// Migrates updated object with finalizer.
@@ -1981,10 +1985,11 @@ func TestHelmRepositoryReconciler_ociMigration(t *testing.T) {
19811985

19821986
g.Eventually(func() bool {
19831987
_ = testEnv.Get(ctx, hrKey, hr)
1984-
return !intpredicates.HelmRepositoryOCIRequireMigration(hr)
1988+
ready := conditions.Get(hr, meta.ReadyCondition)
1989+
return conditions.IsReady(hr) &&
1990+
ready != nil && ready.Reason == sourcev1.NoIndexReason &&
1991+
!intpredicates.HelmRepositoryOCIRequireMigration(hr)
19851992
}, timeout, time.Second).Should(BeTrue())
1986-
g.Expect(conditions.IsReady(hr)).To(BeTrue())
1987-
g.Expect(conditions.Get(hr, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
19881993

19891994
// Migrates deleted object with finalizer.
19901995

@@ -2049,14 +2054,21 @@ func TestHelmRepositoryReconciler_ociReadyCondition(t *testing.T) {
20492054
}
20502055
g.Expect(testEnv.Create(ctx, hr)).ToNot(HaveOccurred())
20512056

2052-
waitForSourceReady(ctx, g, hr, false)
2057+
g.Eventually(func() bool {
2058+
if err := testEnv.Get(ctx, client.ObjectKeyFromObject(hr), hr); err != nil {
2059+
return false
2060+
}
2061+
ready := conditions.Get(hr, meta.ReadyCondition)
2062+
return conditions.IsReady(hr) &&
2063+
ready != nil && ready.Reason == sourcev1.NoIndexReason &&
2064+
hr.GetArtifact() == nil &&
2065+
!intpredicates.HelmRepositoryOCIRequireMigration(hr)
2066+
}, timeout).Should(BeTrue())
20532067

20542068
g.Expect(hr.GetArtifact()).To(BeNil())
20552069
g.Expect(hr.Status.URL).To(BeEmpty())
20562070
g.Expect(controllerutil.ContainsFinalizer(hr, sourcev1.SourceFinalizer)).To(BeFalse())
2057-
g.Expect(conditions.Get(hr, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
20582071
g.Expect(conditions.Get(hr, meta.ReadyCondition).Message).To(ContainSubstring("do not produce an index artifact"))
2059-
g.Expect(intpredicates.HelmRepositoryOCIRequireMigration(hr)).To(BeFalse())
20602072

20612073
// Spec updates on a static Ready object must not wipe the condition.
20622074
patchHelper, err := patch.NewHelper(hr, testEnv.Client)
@@ -2171,3 +2183,66 @@ func TestHelmRepositoryReconciler_migrationToStatic_emptyStatus(t *testing.T) {
21712183
g.Expect(conditions.IsReady(got2)).To(BeTrue())
21722184
g.Expect(conditions.Get(got2, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
21732185
}
2186+
2187+
func TestHelmRepositoryReconciler_migrationToStatic_staleReady(t *testing.T) {
2188+
tests := []struct {
2189+
name string
2190+
markReady func(obj *sourcev1.HelmRepository)
2191+
}{
2192+
{
2193+
name: "Succeeded reason",
2194+
markReady: func(obj *sourcev1.HelmRepository) {
2195+
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "fetched index")
2196+
},
2197+
},
2198+
{
2199+
name: "empty reason",
2200+
markReady: func(obj *sourcev1.HelmRepository) {
2201+
conditions.MarkTrue(obj, meta.ReadyCondition, "", "")
2202+
},
2203+
},
2204+
}
2205+
2206+
for _, tt := range tests {
2207+
t.Run(tt.name, func(t *testing.T) {
2208+
g := NewWithT(t)
2209+
2210+
obj := &sourcev1.HelmRepository{
2211+
ObjectMeta: metav1.ObjectMeta{
2212+
Name: "oci-stale-ready",
2213+
Namespace: "default",
2214+
Generation: 1,
2215+
},
2216+
Spec: sourcev1.HelmRepositorySpec{
2217+
Type: sourcev1.HelmRepositoryTypeOCI,
2218+
URL: "oci://example.com/charts",
2219+
Interval: metav1.Duration{Duration: interval},
2220+
},
2221+
Status: sourcev1.HelmRepositoryStatus{
2222+
ObservedGeneration: 1,
2223+
},
2224+
}
2225+
tt.markReady(obj)
2226+
g.Expect(intpredicates.HelmRepositoryOCIRequireMigration(obj)).To(BeTrue())
2227+
2228+
r := &HelmRepositoryReconciler{
2229+
Client: fakeclient.NewClientBuilder().
2230+
WithScheme(testEnv.GetScheme()).
2231+
WithObjects(obj).
2232+
WithStatusSubresource(&sourcev1.HelmRepository{}).
2233+
Build(),
2234+
EventRecorder: record.NewFakeRecorder(32),
2235+
Storage: testStorage,
2236+
}
2237+
2238+
_, err := r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(obj)})
2239+
g.Expect(err).ToNot(HaveOccurred())
2240+
2241+
got := &sourcev1.HelmRepository{}
2242+
g.Expect(r.Client.Get(context.TODO(), client.ObjectKeyFromObject(obj), got)).To(Succeed())
2243+
g.Expect(conditions.IsReady(got)).To(BeTrue())
2244+
g.Expect(conditions.Get(got, meta.ReadyCondition).Reason).To(Equal(sourcev1.NoIndexReason))
2245+
g.Expect(intpredicates.HelmRepositoryOCIRequireMigration(got)).To(BeFalse())
2246+
})
2247+
}
2248+
}

‎internal/predicates/helmrepository_type_predicate.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,9 @@ func (HelmRepositoryOCIMigrationPredicate) Delete(e event.DeleteEvent) bool {
5555
// returns true.
5656
//
5757
// An OCI object is fully migrated when the source finalizer is gone, no
58-
// leftover index Artifact remains, and Ready=True is set. Empty status is
59-
// not treated as migrated so a one-shot reconcile can set that condition.
58+
// leftover index Artifact remains, and Ready=True is set with reason
59+
// NoIndex. Empty status and stale Ready (for example Succeeded) are not
60+
// treated as migrated so a one-shot reconcile can establish that condition.
6061
func HelmRepositoryOCIRequireMigration(o client.Object) bool {
6162
if o == nil {
6263
return false
@@ -84,10 +85,12 @@ func HelmRepositoryOCIRequireMigration(o client.Object) bool {
8485
}
8586

8687
// isOCIHelmRepositoryStatic reports whether an OCI HelmRepository has completed
87-
// migration to a static object with Ready=True and no leftover index Artifact.
88+
// migration to a static object with Ready=True reason NoIndex and no leftover
89+
// index Artifact.
8890
func isOCIHelmRepositoryStatic(obj *sourcev1.HelmRepository) bool {
8991
if obj.Status.Artifact != nil || obj.Status.URL != "" {
9092
return false
9193
}
92-
return conditions.IsTrue(obj, meta.ReadyCondition)
94+
return conditions.IsTrue(obj, meta.ReadyCondition) &&
95+
conditions.GetReason(obj, meta.ReadyCondition) == sourcev1.NoIndexReason
9396
}

‎internal/predicates/helmrepository_type_predicate_test.go‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func TestHelmRepositoryOCIMigrationPredicate_Create(t *testing.T) {
5858
want: true,
5959
},
6060
{
61-
name: "static oci helm repo with Ready",
61+
name: "static oci helm repo with Ready NoIndex",
6262
beforeFunc: func(o *sourcev1.HelmRepository) {
6363
o.Spec.Type = sourcev1.HelmRepositoryTypeOCI
6464
o.Status = sourcev1.HelmRepositoryStatus{
@@ -68,6 +68,25 @@ func TestHelmRepositoryOCIMigrationPredicate_Create(t *testing.T) {
6868
},
6969
want: false,
7070
},
71+
{
72+
name: "oci helm repo with stale Ready Succeeded",
73+
beforeFunc: func(o *sourcev1.HelmRepository) {
74+
o.Spec.Type = sourcev1.HelmRepositoryTypeOCI
75+
o.Status = sourcev1.HelmRepositoryStatus{
76+
ObservedGeneration: 3,
77+
}
78+
conditions.MarkTrue(o, meta.ReadyCondition, meta.SucceededReason, "fetched index")
79+
},
80+
want: true,
81+
},
82+
{
83+
name: "oci helm repo with Ready empty reason",
84+
beforeFunc: func(o *sourcev1.HelmRepository) {
85+
o.Spec.Type = sourcev1.HelmRepositoryTypeOCI
86+
conditions.MarkTrue(o, meta.ReadyCondition, "", "")
87+
},
88+
want: true,
89+
},
7190
{
7291
name: "oci helm repo with leftover artifact",
7392
beforeFunc: func(o *sourcev1.HelmRepository) {
@@ -136,7 +155,7 @@ func TestHelmRepositoryOCIMigrationPredicate_Update(t *testing.T) {
136155
want: true,
137156
},
138157
{
139-
name: "update static oci repo with Ready",
158+
name: "update static oci repo with Ready NoIndex",
140159
beforeFunc: func(oldObj, newObj *sourcev1.HelmRepository) {
141160
oldObj.Spec = sourcev1.HelmRepositorySpec{
142161
Type: sourcev1.HelmRepositoryTypeOCI,
@@ -148,6 +167,19 @@ func TestHelmRepositoryOCIMigrationPredicate_Update(t *testing.T) {
148167
},
149168
want: false,
150169
},
170+
{
171+
name: "update oci repo with stale Ready Succeeded",
172+
beforeFunc: func(oldObj, newObj *sourcev1.HelmRepository) {
173+
oldObj.Spec = sourcev1.HelmRepositorySpec{
174+
Type: sourcev1.HelmRepositoryTypeOCI,
175+
URL: "oci://foo/bar",
176+
}
177+
conditions.MarkTrue(oldObj, meta.ReadyCondition, meta.SucceededReason, "fetched index")
178+
*newObj = *oldObj.DeepCopy()
179+
newObj.Spec.URL = "oci://foo/baz"
180+
},
181+
want: true,
182+
},
151183
{
152184
name: "migrate old oci repo with leftover artifact",
153185
beforeFunc: func(oldObj, newObj *sourcev1.HelmRepository) {
@@ -262,6 +294,14 @@ func TestHelmRepositoryOCIMigrationPredicate_Delete(t *testing.T) {
262294
},
263295
want: false,
264296
},
297+
{
298+
name: "oci with stale Ready Succeeded",
299+
beforeFunc: func(obj *sourcev1.HelmRepository) {
300+
obj.Spec.Type = sourcev1.HelmRepositoryTypeOCI
301+
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "fetched index")
302+
},
303+
want: true,
304+
},
265305
{
266306
name: "oci without finalizer or status",
267307
beforeFunc: func(obj *sourcev1.HelmRepository) {

0 commit comments

Comments
 (0)