Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add ability to configure and run preflight checks during install, upgrade, and uninstall operations #327

Closed
wants to merge 3 commits into from
Closed

add ability to configure and run preflight checks during install, upgrade, and uninstall operations #327

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13ee5e5
add ability to configure and run preflight checks during install, upg…
everettraven Apr 30, 2024
7bacbdf
formatting
everettraven Apr 30, 2024
bd9d7f3
gofmt
everettraven Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 112 additions & 6 deletions pkg/client/actionclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,13 @@
Reconcile(rel *release.Release) error
}

type GetOption func(*action.Get) error
type InstallOption func(*action.Install) error
type UpgradeOption func(*action.Upgrade) error
type UninstallOption func(*action.Uninstall) error
type RollbackOption func(*action.Rollback) error
type (
GetOption func(*action.Get) error
InstallOption func(*action.Install) error
UpgradeOption func(*action.Upgrade) error
UninstallOption func(*action.Uninstall) error
RollbackOption func(*action.Rollback) error
)

type ActionClientGetterOption func(*actionClientGetter) error

Expand Down Expand Up @@ -115,6 +117,13 @@
}
}

func AppendPreflight(preflights ...Preflight) ActionClientGetterOption {
return func(getter *actionClientGetter) error {
getter.preflights = append(getter.preflights, preflights...)
return nil
}
}

func NewActionClientGetter(acg ActionConfigGetter, opts ...ActionClientGetterOption) (ActionClientGetter, error) {
actionClientGetter := &actionClientGetter{acg: acg}
for _, opt := range opts {
Expand All @@ -125,6 +134,40 @@
return actionClientGetter, nil
}

type Preflight interface {
Run(string, *release.Release) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing a string representing whether it's an install/upgrade/uninstall, could we have a struct that implements separate functions for each phase as a no-op by default where a user can provide "overrides" for just the preflight phases they care about. Basically something something similar to controller-runtime's handler.Funcs

Name() string
}

const (
PreflightOperationInstall = "install"
PreflightOperationUpgrade = "upgrade"
PreflightOperationUninstall = "uninstall"
)

type PreflightFunc func(string, *release.Release) error

func NewPreflightFunc(name string, pf PreflightFunc) Preflight {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to naming suggestions here. I'm thinking something like NewPreflightFromPreflightFunc is more suitable but feels long

Going to spend some time looking into how other packages tend to do this to get some inspiration but figured I shouldn't hold getting a PR up for review on this.

return &preflightFuncImpl{

Check failure on line 151 in pkg/client/actionclient.go

View workflow job for this annotation

GitHub Actions / Lint 

File is not `gofmt`-ed with `-s` (gofmt)
preflightFunc: pf,
name: name,
}
}

type preflightFuncImpl struct {
preflightFunc PreflightFunc
name string
}

func (pfi *preflightFuncImpl) Run(operation string, rel *release.Release) error {
return pfi.preflightFunc(operation, rel)
}

func (pfi *preflightFuncImpl) Name() string {
return pfi.name
}


type actionClientGetter struct {
acg ActionConfigGetter

Expand All @@ -136,6 +179,8 @@
installFailureUninstallOpts []UninstallOption
upgradeFailureRollbackOpts []RollbackOption

preflights []Preflight

postRendererProviders []PostRendererProvider
}

Expand All @@ -150,7 +195,7 @@
if err != nil {
return nil, err
}
var cpr = chainedPostRenderer{}
cpr := chainedPostRenderer{}
for _, provider := range hcg.postRendererProviders {
cpr = append(cpr, provider(rm, actionConfig.KubeClient, obj))
}
Expand All @@ -167,6 +212,8 @@
defaultUpgradeOpts: append([]UpgradeOption{WithUpgradePostRenderer(cpr)}, hcg.defaultUpgradeOpts...),
defaultUninstallOpts: hcg.defaultUninstallOpts,

preflights: hcg.preflights,

installFailureUninstallOpts: hcg.installFailureUninstallOpts,
upgradeFailureRollbackOpts: hcg.upgradeFailureRollbackOpts,
}, nil
Expand All @@ -180,6 +227,8 @@
defaultUpgradeOpts []UpgradeOption
defaultUninstallOpts []UninstallOption

preflights []Preflight

installFailureUninstallOpts []UninstallOption
upgradeFailureRollbackOpts []RollbackOption
}
Expand All @@ -205,6 +254,25 @@
}
install.ReleaseName = name
install.Namespace = namespace

// TODO: Open Question - should we always run preflight checks if they exist?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions here.

My thinking behind not running the preflight checks if dry run is specified is because the caller may have a certain expectation of the output with a dry run and including preflight checks in that could pollute the dry run results.

if len(c.preflights) > 0 && !install.DryRun {
install.DryRun = true
c.conf.Log("Running preflight checks")
c.conf.Log("Performing install dry run")
rel, err := install.Run(chrt, vals)
if err != nil {
c.conf.Log("Install dry run failed")
return nil, fmt.Errorf("preflight checks were specified, install dry run failed: %w", err)
}
for _, preflight := range c.preflights {
if err := preflight.Run(PreflightOperationInstall, rel); err != nil {
return nil, fmt.Errorf("preflight check %q failed: %w", preflight.Name(), err)
}
}
install.DryRun = false
}

c.conf.Log("Starting install")
rel, err := install.Run(chrt, vals)
if err != nil {
Expand Down Expand Up @@ -241,6 +309,25 @@
}
}
upgrade.Namespace = namespace

// TODO: Open Question - should we always run preflight checks if they exist?
if len(c.preflights) > 0 && !upgrade.DryRun {
upgrade.DryRun = true
c.conf.Log("Running preflight checks")
c.conf.Log("Performing upgrade dry run")
rel, err := upgrade.Run(name, chrt, vals)
if err != nil {
c.conf.Log("Upgrade dry run failed")
return nil, fmt.Errorf("preflight checks were specified, upgrade dry run failed: %w", err)
}
for _, preflight := range c.preflights {
if err := preflight.Run(PreflightOperationUpgrade, rel); err != nil {
return nil, fmt.Errorf("preflight check %q failed: %w", preflight.Name(), err)
}
}
upgrade.DryRun = false
}

rel, err := upgrade.Run(name, chrt, vals)
if err != nil {
if rel != nil {
Expand Down Expand Up @@ -286,6 +373,25 @@
return nil, err
}
}

// TODO: Open Question - should we always run preflight checks if they exist?
if len(c.preflights) > 0 && !uninstall.DryRun {
uninstall.DryRun = true
c.conf.Log("Running preflight checks")
c.conf.Log("Performing uninstall dry run")
rel, err := uninstall.Run(name)
if err != nil {
c.conf.Log("Uninstall dry run failed")
return nil, fmt.Errorf("preflight checks were specified, uninstall dry run failed: %w", err)
}
for _, preflight := range c.preflights {
if err := preflight.Run(PreflightOperationUninstall, rel.Release); err != nil {
return nil, fmt.Errorf("preflight check %q failed: %w", preflight.Name(), err)
}
}
uninstall.DryRun = false
}

return uninstall.Run(name)
}

Expand Down
Loading
Loading