-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feat async install helm #97
base: main
Are you sure you want to change the base?
Conversation
pkg/service/HelmAppService.go
Outdated
} | ||
_ = impl.pubsubClient.Publish(pubsub_lib.HELM_CHART_INSTALL_STATUS_TOPIC, string(data)) | ||
}() | ||
rel, err := helmClientObj.InstallChart(context.Background(), chartSpec) |
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 should be helmClientObj.InstallChart(ctx, chartSpec)
} else { | ||
impl.logger.Errorw("Error in upgrade release with chart info", "err", err) | ||
impl.logger.Debug("Upgrading release with chart info") | ||
_, err = helmClientObj.UpgradeReleaseWithChartInfo(ctx, chartSpec) |
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.
as the async install/upgrade flow is flag driven, we should expose a flag in our gRPC payload (e.g: runInContext: bool) and use native context/ context.background() accordingly. The current flow will break for sync deployments as it will run with a default timeout now.
pkg/service/HelmAppService.go
Outdated
_, err = helmClientObj.UpgradeReleaseWithChartInfo(ctx, chartSpec) | ||
if UpgradeErr, ok := err.(*driver.StorageDriverError); ok { | ||
if UpgradeErr != nil { | ||
if UpgradeErr.Err == driver.ErrReleaseNotFound { |
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.
instead of driver.ErrReleaseNotFound, it should be driver.ErrNoDeployedReleases
…ll-flag fixed: helm upgrade with install flag for helm apps
This reverts commit 07d207b.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
No description provided.