⚠ minor API improvements #1250
⚠ minor API improvements #1250
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
joelanford
force-pushed
the
minor-api-updates
branch
from
7c3772e to
8e31236
Compare
| @@ -23,8 +23,7 @@ import ( | |||
| ) | |||
|
|
|||
| var ( | |||
| ClusterExtensionGVK = SchemeBuilder.GroupVersion.WithKind("ClusterExtension") | |||
There was a problem hiding this comment.
ClusterExtensionGVK is unused, and is not really necessary for inclusion as an exported global variable in the API.
Callers can construct themselves like this:
gvk := ocv1alpha1.GroupVersion.WithKind(v1alpha1.ClusterExtensionKind)
| // - "Unpacked", represents whether or not the bundle contents have been successfully unpacked | ||
| // | ||
| // When the ClusterExtension is sourced from a catalog, the following conditions are also possible: |
There was a problem hiding this comment.
I grouped the Deprecated* conditions into their own section for two reasons:
- I think it is helpful to see the other "main" conditions without the "noise" of these being intermingled.
- Deprecation metadata comes from catalogs, and while we currently only support sourcing from catalogs, I thought it might be good to call out the fact that deprecations will only ever show up if the ClusterExtension is sourced from a catalog.
| // - "Deprecated", represents an aggregation of the PackageDeprecated, ChannelDeprecated, and BundleDeprecated condition types | ||
| // - "PackageDeprecated", represents whether or not the package specified in the spec.source.catalog.packageName field has been deprecated | ||
| // - "ChannelDeprecated", represents whether or not any channel specified in spec.source.catalog.channels has been deprecated | ||
| // - "BundleDeprecated", represents whether or not the installed bundle is deprecated |
There was a problem hiding this comment.
I'm pretty sure this line is not true, but want to double check. A quick glance at the code makes me think we should make the following edit as well:
| // - "BundleDeprecated", represents whether or not the installed bundle is deprecated | |
| // - "BundleDeprecated", represents whether or not the resolved bundle is deprecated |
However, I think the original sentiment of having this condition be based on the installed bundle is what we actually want. That might require a bit more conversation and consideration though, as it isn't clear to me how exactly we would assess the deprecation status of an installed bundle during subsequent reconciles after it is installed.
There was a problem hiding this comment.
I guess we'd have two possibilities:
- You want to install a bundle from the catalog, and it's deprecated - which is straightforward
- You install a bundle that isn't deprecated, and in the next catalog version it is (I guess this is only meaningful for version ranges that don't let the bundle upgrade to the next (non-deprecated) version.
In the 2nd case, shouldn't it pick up the condition during reconcile? Maybe it's OK that we only express deprecated conditions for resolved bundles (and not worry about the the installed bundles) since either the CE will get upgraded, or if it can't/won't, we'd anyway resolve to the installed bundle?
There was a problem hiding this comment.
My initial thought is that we only touch the BundleDeprecated condition when:
- We successfully install a bundle. In this case the resolved bundle and the installed bundle are the same, so the resolved deprecation is applicable to the installed bundle.
- We detect that a bundle is somehow no longer installed, in which case we unset (or set to
Unknown) theBundleDeprecatedcondition.
| // | ||
| //+optional | ||
| Bundle *BundleMetadata `json:"bundle,omitempty"` | ||
| Bundle BundleMetadata `json:"bundle"` |
There was a problem hiding this comment.
I removed the pointer here because the parent struct is a pointer. If the parent struct is non-nil, this field is always expected to be set.
| Bundle *BundleMetadata `json:"bundle,omitempty"` | ||
| // A "bundle" is a versioned set of content that represents the resources that | ||
| // need to be applied to a cluster to install a package. | ||
| Bundle BundleMetadata `json:"bundle"` |
There was a problem hiding this comment.
joelanford
force-pushed
the
minor-api-updates
branch
from
8e31236 to
1a4c1d4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1250 +/- ##
==========================================
- Coverage 76.49% 76.18% -0.32%
==========================================
Files 40 40
Lines 2336 2330 -6
==========================================
- Hits 1787 1775 -12
- Misses 392 398 +6
Partials 157 157
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
joelanford
force-pushed
the
minor-api-updates
branch
2 times, most recently
from
dc80a94 to
a21d458
Compare
Signed-off-by: Joe Lanford <[email protected]>
joelanford
force-pushed
the
minor-api-updates
branch
from
a21d458 to
13d5309
Compare
| - "PackageDeprecated", represents whether or not the package specified in the spec.source.catalog.packageName field has been deprecated | ||
| - "ChannelDeprecated", represents whether or not any channel specified in spec.source.catalog.channels has been deprecated | ||
| - "BundleDeprecated", represents whether or not the installed bundle is deprecated | ||
|
|
||
| The current set of reasons are: | ||
| - "Success", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when unpacking a bundle's content, resolution and installation/upgrading is successful |
There was a problem hiding this comment.
Do we need to call out the reasons for the deprecated conditions?
There was a problem hiding this comment.
I don't think so. The "Reason" for these conditions are basically 1-to-1 with the Status. "The reason this is deprecated is because the author provided a deprecation"
f61ffb4
Description
More minor improvements to the API in preparation for 1.0.0.
Fixes #1251
Reviewer Checklist