[SHIPA-2322] adds wait-retry to helm update & delete #233
Conversation
clean up tests
30f592f to
894e26f
Compare
internal/chart/helm_client.go
Outdated
| type statusFunc func(cfg *action.Configuration, appName string) (*release.Release, release.Status, error) | ||
|
|
||
| const ( | ||
| WaitRetry = iota |
There was a problem hiding this comment.
it looks like there is no need to export WaitRetry, TakeAction and NoAction.
Is it possible to use waitRetry, takeAction and noAction correspondingly?
There was a problem hiding this comment.
You bet. Privatized!
internal/chart/helm_client.go
Outdated
|
|
||
| // helmStatusActionMapUpdate maps a Release Status to a Ketch action for helm updates | ||
| var helmStatusActionMapUpdate = map[release.Status]int{ | ||
| "not-found": TakeAction, |
There was a problem hiding this comment.
maybe a new const for not-found?
internal/chart/helm_client.go
Outdated
| func (c HelmClient) waitForActionableStatus(statusFunc statusFunc, appName string, statusActionMap map[release.Status]int) (bool, error) { | ||
| ticker := time.NewTicker(statusRetryInterval) | ||
| done := time.After(statusRetryTimeout) | ||
| var helmRelease *release.Release |
There was a problem hiding this comment.
I like this approach, but will it block the appReconciler's loop?
if yes, what are the negative consequences we are going to deal with?
There was a problem hiding this comment.
Per our slack discussion: I removed the wait loop. Now, "wait-retry" statuses throw an error, which the reconciler loop is expected to handle. This means that 1) we don't manually update a chart's status here (not sure if that's good or bad) 2) there is a possiblity that a chart will get stuck in a weird status like pending-uninstall and the reconciler will just keep re-trying. Not sure if that's something to be concerned about.
| return false, nil | ||
| case takeAction: | ||
| return true, nil | ||
| default: |
There was a problem hiding this comment.
would it help if we implement something like
ketch/internal/chart/helm_client.go
Lines 129 to 136 in b9e8b85
i
There was a problem hiding this comment.
Yes, that makes sense. This PR's addition almost duplicates that FirstDeployed.Before check, but considers additional statuses. I removed the original block of code.
Description
2322
Helm update and helm delete currently fail when the object status (app) is not
deployed. This change checks a map for actionable & retryable statuses for updates and deletions. For wait-retry statuses, a wait-retry loop is entered, and ultimately the status is manually updated todeployed.Type of change
Testing
Documentation
Final Checklist: