-
Notifications
You must be signed in to change notification settings - Fork 47
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
✨ remove extension-specific type checks #842
✨ remove extension-specific type checks #842
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #842 +/- ##
==========================================
+ Coverage 71.02% 71.70% +0.68%
==========================================
Files 17 18 +1
Lines 1301 1329 +28
==========================================
+ Hits 924 953 +29
+ Misses 304 300 -4
- Partials 73 76 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
// skipCRDUpgradeSafetyCheck specifies whether the CRD upgrade safety checks should be skipped when attempting to install the ClusterExtension | ||
SkipCRDUpgradeSafetyCheck bool `json:"skipCRDUpgradeSafetyCheck,omitempty"` |
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 @rashmigottipati is working on some changes that are going to be more extensible for future preflight checks. I think it would be okay to keep this field out of these changes
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.
Oh, I see - this PR still has the extension type. I forgot #820 has not merged yet :)
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.
Ignore the previous comment. I accidentally commented this on the wrong review thread.
config/admission/admission.yaml
Outdated
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.
// Don't do anything if Paused | ||
ext.Status.Paused = ext.Spec.Paused | ||
if ext.Spec.Paused { | ||
l.Info("resource is paused", "name", ext.GetName(), "namespace", ext.GetNamespace()) | ||
return ctrl.Result{}, nil | ||
} |
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 know that in theory we shouldn't do anything, but from a UX perspective I think it might be worth setting some form of status indicating that "no work was done" because it is paused.
What do you think of something like setting Progressing
to False
with a reason Paused
?
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'm also OK with this being done in a follow-up since the scope of this PR is to make the two APIs consistent
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.
A couple comments relating to existing or future PRs that are modifying some of the same things as a heads up and a suggestion for UX on paused state so users don't have to check the logs to verify that a resource is in the paused state.
Since the scope of this PR is to bring the Extension
and ClusterExtension
APIs to parity I think that it is OK to merge this without addressing any of my comments and implementing my pause suggestion as a follow-up.
Great work @ankitathomas !
type ClusterExtensionSource struct { | ||
//+kubebuilder:validation:Enum:=package | ||
//+kubebuilder:validation:Required | ||
// SourceType is the discriminator for the source type | ||
SourceType string `json:"sourceType"` | ||
|
||
// Package defines a reference for a bundle in a catalog defined by a name and a version and/or channel | ||
Package *SourcePackage `json:"package,omitempty"` | ||
} |
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.
// Lookup the bundle that corresponds to the ClusterExtension's desired package. | ||
bundle, err := r.resolve(ctx, ext) | ||
if err != nil { | ||
ext.Status.InstalledBundle = nil | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration()) | ||
if c := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1alpha1.TypeInstalled); c == nil { |
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.
Do we need this additional check for finding status condition? IIRC Installed status is added by default. I see that we are not doing this for bundle validation or unpack, so to maintain consistency, it's better to rather remove this check here, and explicitly set installed to nil regardless.
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.
We only need the status check if we want to preserve the installed condition from the currently installed thing in case of a resolution failure to indicate that the installed version is healthy even if there's an error on the Extension/ClusterExtension. We probably don't need this check once we have a separate healthy condition the users could look at instead.
@@ -155,6 +167,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp | |||
ext.Status.InstalledBundle = nil | |||
setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | |||
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration()) | |||
setProgressingStatusConditionProgressing(&ext.Status.Conditions, "installation failed", ext.GetGeneration()) |
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.
If installed is explicitly set to failed, and we are not able to find if the bundle deployment exists on cluster or not, I think this should be set to False
instead, since there is no other state of improvement after this. Was there a specific reason to make it true instead?
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 thought of the progressing condition as more of an indication of 'an install/upgrade is in progress, the currently installed bundle may be unstable'. Since it's stuck progressing here, it should be an indicator that the current installation is possibly incomplete/broken.
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration()) | ||
setProgressingStatusConditionProgressing(&ext.Status.Conditions, "installation failed", ext.GetGeneration()) |
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.
The same comment as above, why set Progressing as true and not as false when we are returning an error here and know that we cannot proceed with install?
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.
Thinking about it, my previous comment may with respect to rukpak API may not be relevant when we get Helm stuff in, but this particular case would still hold true.
Please hold this from merging till #846 gets in. Thanks! |
/hold |
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
a934c27
to
27ad230
Compare
No description provided.