-
Notifications
You must be signed in to change notification settings - Fork 53
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
✨ Add checks for helm deployment status #1349
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 #1349 +/- ##
==========================================
- Coverage 76.42% 74.55% -1.88%
==========================================
Files 41 42 +1
Lines 2431 2515 +84
==========================================
+ Hits 1858 1875 +17
- Misses 400 453 +53
- Partials 173 187 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
case rel.Info.Status == release.StatusDeployed: | ||
case rel.Info.Status == release.StatusUninstalled: | ||
case rel.Info.Status == release.StatusSuperseded: | ||
case rel.Info.Status == release.StatusFailed: |
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 the release is failed, do we want to allow it to be overridden by default?
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.
Based on my understanding of #1296 I would assume we would not want to do this.
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.
It's in a terminal state; and if an upgrade is the only possible way to fix it, then we need to allow it to happen. Otherwise, the installation will remain in a failed state and be unable to be replaced without manual intervention (i.e. removal).
switch { | ||
case rel.Info.Status == release.StatusUnknown: | ||
return nil, fmt.Errorf("installation status is unknown") | ||
case rel.Info.Status == release.StatusDeployed: | ||
case rel.Info.Status == release.StatusUninstalled: | ||
case rel.Info.Status == release.StatusSuperseded: | ||
case rel.Info.Status == release.StatusFailed: | ||
case rel.Info.Status == release.StatusUninstalling: | ||
return nil, fmt.Errorf("installation is still pending: %s", rel.Info.Status) | ||
case rel.Info.Status.IsPending(): | ||
return nil, fmt.Errorf("installation is still pending: %s", rel.Info.Status) | ||
default: | ||
return nil, fmt.Errorf("unknown installation status: %s", rel.Info.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.
If the status of the release is "Terminal", then we ought to be able to upgrade etc. If it is unknown (in the multiple ways that unknown is defined, ha!) then it is considered an error, The default
case catches the situation when the helm API changes. Only if something is pending (and for some reason, StatusUninstalling
is not considered pending), then should we wait.
Don't allow an update to occur when the deployment is still installing. Signed-off-by: Todd Short <tshort@redhat.com>
ping @thetechnick |
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.
/lgtm
8558349
Fix #1296
Don't allow an update to occur when the deployment is still installing.
Description
Reviewer Checklist