🐛(fix) Remove "Serving" condition type from ConditionSets #1859
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
anik120
changed the title
anik120
force-pushed
the
clear-serving-error
branch
from
40e213b to
2db9364
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1859 +/- ##
==========================================
- Coverage 68.45% 68.39% -0.06%
==========================================
Files 63 63
Lines 5136 5136
==========================================
- Hits 3516 3513 -3
- Misses 1390 1392 +2
- Partials 230 231 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -61,7 +61,7 @@ func parseConstants(prefix string) ([]string, error) { | |||
| // ParseDir returns a map of package name to package ASTs. An AST is a representation of the source code | |||
| // that can be traversed to extract information. The map is keyed by the package name. | |||
| pkgs, err := parser.ParseDir(fset, ".", func(info fs.FileInfo) bool { | |||
| return !strings.HasSuffix(info.Name(), "_test.go") | |||
| return !strings.HasSuffix(info.Name(), "_test.go") && !(info.Name() == "clustercatalog_types.go") | |||
There was a problem hiding this comment.
Maybe we should test that the filename is clusterextension_types.go instead?
That way if we ever add a new type, we don't need to change this to exclude that new file?
There was a problem hiding this comment.
That's a good point. I actually changed the function to
// parseConstants parses the values of the top-level constants that start with the given prefix,
// in the files clusterextension_types.go and common_types.go.
func parseConstants(prefix string) ([]string, error)
so that this function never sees any movement if new types are added. It also helped to get rid of the 3 nest for-loops and replace it with a nicer looking 2 nested one.
All this made possible by the replacement of parseDir with parseFile
api/v1/clustercatalog_types.go
Outdated
| @@ -34,6 +34,14 @@ const ( | |||
|
|
|||
| AvailabilityModeAvailable AvailabilityMode = "Available" | |||
| AvailabilityModeUnavailable AvailabilityMode = "Unavailable" | |||
|
|
|||
| // Serving condition types | |||
There was a problem hiding this comment.
nit: Serving Condition types?
There was a problem hiding this comment.
Done 👍🏽
I pushed a change to address Joe's feedback too can I get a re-lgtm please 🙏🏽
anik120
force-pushed
the
clear-serving-error
branch
from
2db9364 to
56397fd
Compare
anik120
force-pushed
the
clear-serving-error
branch
3 times, most recently
from
6f12f28 to
da50278
Compare
api/v1/clustercatalog_types.go
Outdated
| // Serving Condition types | ||
| TypeServing = "Serving" |
There was a problem hiding this comment.
I think this is what @azych meant? If we add more condition types, we can add them in this section.
| // Serving Condition types | |
| TypeServing = "Serving" | |
| // Condition types | |
| TypeServing = "Serving" |
But I would leave Serving reasons as its own section since it can be helpful to see which reasons are used for which condition types.
|
Other then the nit on the comment, changes look good to me! |
|
/lgtm |
conditionset.ConditionTypes was being used by the CluterExtension
controller to `ensureAllConditionsWithReason` whenever a particular
reason needed to be set for all of the ClusterExention's conditions.
However, the `ConditionTypes` included a Condition Type `Serving`, which
is not a ClusterExtension Condition(it is a ClusterCatalog condition).
This is causing the `Serving` condition to show up when a resolution fails,
which is incorrect to begin with, and is then never cleared when the resolution
succeeds at a later stage.
```
try to upgrade ClusterExtension to a non-exist version
$ kubectl patch ClusterExtension extension-77972 -p '{"spec":{"source":{"catalog":{"version":"0.2.0"}}}}' --type=merge
clusterextension.olm.operatorframework.io/extension-77972 patched
- lastTransitionTime: "2025-03-04T07:16:27Z"
message: 'error upgrading from currently installed version "0.1.0": no bundles
found for package "nginx77972" matching version "0.2.0"'
observedGeneration: 2
reason: Retrying
status: "True"
type: Progressing
- lastTransitionTime: "2025-03-04T07:16:35Z"
message: 'error upgrading from currently installed version "0.1.0": no bundles
found for package "nginx77972" matching version "0.2.0"'
observedGeneration: 2
reason: Failed
status: "False"
type: Serving
install:
bundle:
name: nginx77972.v0.1.0
version: 0.1.0
```
This PR removes the `Serving` condition type from `conditonSets.ConditionType`
anik120
force-pushed
the
clear-serving-error
branch
from
da50278 to
46d0ca6
Compare
|
New changes are detected. LGTM label has been removed. |
029d19f
conditionset.ConditionTypes was being used by the CluterExtension controller to
ensureAllConditionsWithReasonwhenever a particular reason needed to be set for all of the ClusterExention's conditions. However, theConditionTypesincluded a Condition TypeServing, which is not a ClusterExtension Condition(it is a ClusterCatalog condition).This is causing the
Servingcondition to show up when a resolution fails, which is incorrect to begin with, and is then never cleared when the resolution succeeds at a later stage.This PR removes the
Servingcondition type fromconditonSets.ConditionTypeDescription
Reviewer Checklist