-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
⚠ Status condition clean up #379
⚠ Status condition clean up #379
Conversation
58d5bcc
to
b64388b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #379 +/- ##
==========================================
- Coverage 37.25% 36.05% -1.20%
==========================================
Files 14 14
Lines 816 796 -20
==========================================
- Hits 304 287 -17
Misses 463 463
+ Partials 49 46 -3 ☔ View full report in Codecov by Sentry. |
b64388b
to
0b0c5dd
Compare
0b0c5dd
to
78454fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work!
There are a few more mentions of the Unpacking condition we should probably clean up in the README.md, docs/fetching-catalog-contents.md, test/tools/imageregistry/pre-upgrade-setup.sh and hack/scripts/gzip-demo-script.sh and hack/scripts/demo-script.sh.
We'd want to at least update the pre-upgrade-setup script since that's part of upgrade testing, but it'd be good to keep everything else up to date
78454fb
to
e8e2fd3
Compare
Thanks for pointing this out @ankitathomas ! I updated most of it, except for the Or did I get that wrong? The pre-upgrade test is passing with this setting though.... Also, I'll look into the updating of the actual demo file after lunch, but for now I ran into this issue, not sure if @grokspawn can help
|
Everything looks good!
If we're ok with the test breaking before we're done with the API review work, it may be easier to have this change on your PR and cut a new release immediately after, but you're right - we can also update the upgrade-e2e script in a followup PR if we want to keep the test passing. |
This looks like your bash env wasn't able to test if the tempdir exists here. |
IMO, we're making these changes in full awareness that we are breaking the API. We have opted not to create an interim API version to avoid it, and in the past we've committed and tried to cut a new release of the component to get the test passing again. |
7760e8d
to
7d2f636
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes LGTM, but we should probably get @grokspawn and @joelanford to do a final pass as well prior to merging
@ankitathomas fyi I removed the changes to the demo scripts and related stuff. The cognitive load required for review was probably getting a little heavier because of those changes, at least for me. We'll most definitely have more high quality reviews on the changes you want to make there if we put it up as a separate PR anyway. |
@ankitathomas @joelanford @everettraven ready for review again. |
} | ||
updateStatusNotServing(&catalog.Status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a comment here that talks about the issue that we encountered when we both modified the status and the ClusterCatalog
finalizers? I think that issue still exists, we just won't hot-loop because of Joe's change and we would end up setting the status to the same thing (i.e no change) so we end up successfully removing the finalizer on the second reconcile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #409. I don't think we need a TODO comment here besides the issue
3167ee5
to
d902ad3
Compare
d902ad3
to
fa7b181
Compare
fa7b181
to
5101830
Compare
750f545
to
32dd761
Compare
|
[RFC: ClusterCatalog Status Conditions](https://docs.google.com/document/d/1Kg8ovX8d25OqGa5utwcKbX3nN3obBl2KIYJHYz6IlKU/edit) implementation. Closes [363](operator-framework#363) Closes [364](operator-framework#364) Closes [365](operator-framework#365) Co-authored-by: Ankita Thomas <[email protected]> Co-authored-by: Anik Bhattacharjee <[email protected]>
Signed-off-by: Todd Short <[email protected]>
32dd761
to
a7a03d2
Compare
@@ -308,10 +280,13 @@ func NewFinalizers(localStorage storage.Instance, unpacker source.Unpacker) (crf | |||
panic("could not convert object to clusterCatalog") | |||
} | |||
if err := localStorage.Delete(catalog.Name); err != nil { | |||
return crfinalizer.Result{}, updateStatusStorageDeleteError(&catalog.Status, err) | |||
updateStatusProgressing(catalog, err) | |||
return crfinalizer.Result{}, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to return crfinalizer.Result{StatusUpdated: true}
on any code path that changes the status conditions. Otherwise, I'm not sure we fall into the if finalizeResult.Updated || finalizeResult.StatusUpdated
block in line 130.
Signed-off-by: Todd Short <[email protected]>
2dc04d9
RFC: ClusterCatalog Status Conditions implementation.
Closes 363 Closes 364 Closes 365