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.

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 ability to configure and run preflight checks during install, upgrade, and uninstall operations #327

Closed

Conversation

everettraven
Copy link
Contributor

Description:

  • Adds a new Preflight interface and ability to configure a slice of Preflights to run during the actionClient install, upgrade, and uninstall operations.
  • Updates actionClient unit tests to test the preflight check implementation

Motivation:

…rade, and uninstall operations

Signed-off-by: everettraven <everettraven@gmail.com>
Signed-off-by: everettraven <everettraven@gmail.com>

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.

@@ -205,6 +254,25 @@ func (c *actionClient) Install(name, namespace string, chrt *chart.Chart, vals m
}
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.

Signed-off-by: everettraven <everettraven@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 74.00000% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 85.31%. Comparing base (08ab7fb) to head (bd9d7f3).
Report is 14 commits behind head on main.

Files Patch % Lines
pkg/client/actionclient.go 74.00% 10 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
+ Coverage   85.06%   85.31%   +0.24%     
==========================================
  Files          19       19              
  Lines        1346     1559     +213     
==========================================
+ Hits         1145     1330     +185     
- Misses        125      149      +24     
- Partials       76       80       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -125,6 +134,39 @@ func NewActionClientGetter(acg ActionConfigGetter, opts ...ActionClientGetterOpt
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

@everettraven
Copy link
Contributor Author

Closing in favor of putting this directly in operator-controller/rukpak

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants