✨ Adding tests for catalog availability #436
Conversation
LalatenduMohanty
force-pushed
the
tests_for_availability
branch
from
85c44f2 to
1274c47
Compare
LalatenduMohanty
changed the title
LalatenduMohanty
force-pushed
the
tests_for_availability
branch
2 times, most recently
from
927984c to
fbc71e4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
=======================================
Coverage 37.83% 37.83%
=======================================
Files 15 15
Lines 1208 1208
=======================================
Hits 457 457
Misses 701 701
Partials 50 50 ☔ View full report in Codecov by Sentry. |
| { | ||
| Type: catalogdv1alpha1.TypeProgressing, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: catalogdv1alpha1.ReasonDisabled, |
There was a problem hiding this comment.
I was expecting that the status would change but it is not changing which is weird.
There was a problem hiding this comment.
The first reconciliation after the Availability is changed back to Enabled is going to add the finalizers back here. The second reconciliation that'll happen AFTER the finalizer is added will be updating these conditions after finishing its job.
One way we can handle this is probably by breaking this test into two tests
- "ClusterCatalog previously disabled, is enabled back, finalizers are added back"
- "ClusterCatalog previously disabled, is enabled back, finalizers have been added back, status conditions updated to show Serving=True, Progressing=False (or whatever's appropriate)"
There was a problem hiding this comment.
This is a good idea. let me try that. Thanks.
There was a problem hiding this comment.
Could you added a little bit of Description in the commit message for the PR too?(besides the main title). It'll help out someone who hasn't seen the previous PRs about Availability (so the words "catalog availability" doesn't mean much to them yet).
I like creating paper trails. Eg
"This is a follow up to PR #___, that added a new field Availability".
Hopefully the first PR had a link to the RFC, in which case the paper trail would be complete :)
| @@ -736,6 +736,89 @@ func TestCatalogdControllerReconcile(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
| name: "catalog availability set to enable, status should get updated", | |||
There was a problem hiding this comment.
Can we be more clear about what exactly we expect the status to look like? It doesn't have to be very detailed, but a little more clarity will be helpful.
For eg, this one says status updated to reflect terminal error state(Blocked) and error is returned, this one says should reflect in status that it's not progressing anymore, and is serving, etc.
| { | ||
| Type: catalogdv1alpha1.TypeProgressing, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: catalogdv1alpha1.ReasonDisabled, |
There was a problem hiding this comment.
The first reconciliation after the Availability is changed back to Enabled is going to add the finalizers back here. The second reconciliation that'll happen AFTER the finalizer is added will be updating these conditions after finishing its job.
One way we can handle this is probably by breaking this test into two tests
- "ClusterCatalog previously disabled, is enabled back, finalizers are added back"
- "ClusterCatalog previously disabled, is enabled back, finalizers have been added back, status conditions updated to show Serving=True, Progressing=False (or whatever's appropriate)"
Yes, good call out. Will fix that.
Not a fan of mentioning google docs in the PRs specially when they are not 100% public docs. RFC is only shared with upstream OLM groups but the code most likely would end up in places where folks wont be part of the OLM group. |
This is a follow-up from the previous PR operator-framework#421 i.e. commit d320249 Signed-off-by: Lalatendu Mohanty <[email protected]>
LalatenduMohanty
force-pushed
the
tests_for_availability
branch
from
fbc71e4 to
a4881de
Compare
|
One test is panicking currently, so looking in to it https://github.com/operator-framework/catalogd/actions/runs/11524939322/job/32086163161?pr=436 |
This is a follow-up from the previous PR #421 i.e. commit d320249