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 a default value for MaxReleaseHistory #291

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Add a default value for MaxReleaseHistory #291

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
924a2bd
default MaxReleaseHistory to ten
Jay-Madden Jan 30, 2024
521b766
make default history nullable
Jay-Madden Jan 30, 2024
a591ffe
rename maxHistory
Jay-Madden Jan 30, 2024
4523eef
add gomega pointto in tests
Jay-Madden Feb 26, 2024
4295f8e
Merge branch 'main' into add-max-history
joelanford Jul 8, 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
2 changes: 2 additions & 0 deletions pkg/reconciler/internal/values/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import (
"github.com/operator-framework/helm-operator-plugins/pkg/values"
)

var DefaultMaxReleaseHistory = 10

var DefaultMapper = values.MapperFunc(func(v chartutil.Values) chartutil.Values { return v })

var DefaultTranslator = values.TranslatorFunc(func(ctx context.Context, u *unstructured.Unstructured) (chartutil.Values, error) {
Expand Down
21 changes: 14 additions & 7 deletions pkg/reconciler/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ type Reconciler struct {
skipDependentWatches bool
maxConcurrentReconciles int
reconcilePeriod time.Duration
maxHistory int
maxReleaseHistory *int
skipPrimaryGVKSchemeRegistration bool
controllerSetupFuncs []ControllerSetupFunc

Expand Down Expand Up @@ -348,13 +348,15 @@ func WithReconcilePeriod(rp time.Duration) Option {
}

// WithMaxReleaseHistory specifies the maximum size of the Helm release history maintained
// on upgrades/rollbacks. Zero (default) means unlimited.
// on upgrades/rollbacks. Zero means unlimited.
//
// Defaults is 10
func WithMaxReleaseHistory(maxHistory int) Option {
return func(r *Reconciler) error {
if maxHistory < 0 {
return errors.New("maximum Helm release history size must not be negative")
}
r.maxHistory = maxHistory
r.maxReleaseHistory = &maxHistory
return nil
}
}
Expand Down Expand Up @@ -746,9 +748,9 @@ func (r *Reconciler) getReleaseState(client helmclient.ActionInterface, obj meta
}

var opts []helmclient.UpgradeOption
if r.maxHistory > 0 {
if *r.maxReleaseHistory > 0 {
opts = append(opts, func(u *action.Upgrade) error {
u.MaxHistory = r.maxHistory
u.MaxHistory = *r.maxReleaseHistory
return nil
})
}
Expand Down Expand Up @@ -802,9 +804,9 @@ func (r *Reconciler) doInstall(actionClient helmclient.ActionInterface, u *updat

func (r *Reconciler) doUpgrade(actionClient helmclient.ActionInterface, u *updater.Updater, obj *unstructured.Unstructured, vals map[string]interface{}, log logr.Logger) (*release.Release, error) {
var opts []helmclient.UpgradeOption
if r.maxHistory > 0 {
if *r.maxReleaseHistory > 0 {
opts = append(opts, func(u *action.Upgrade) error {
u.MaxHistory = r.maxHistory
u.MaxHistory = *r.maxReleaseHistory
return nil
})
}
Expand Down Expand Up @@ -936,6 +938,11 @@ func (r *Reconciler) addDefaults(mgr ctrl.Manager, controllerName string) error
if r.valueMapper == nil {
r.valueMapper = internalvalues.DefaultMapper
}

if r.maxReleaseHistory == nil {
r.maxReleaseHistory = &internalvalues.DefaultMaxReleaseHistory
}

return nil
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/reconciler/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/go-logr/logr"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
. "github.com/onsi/gomega/gstruct"
"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chartutil"
Expand Down Expand Up @@ -217,11 +218,11 @@ var _ = Describe("Reconciler", func() {
var _ = Describe("WithMaxReleaseHistory", func() {
It("should set the max history size", func() {
Expect(WithMaxReleaseHistory(10)(r)).To(Succeed())
Expect(r.maxHistory).To(Equal(10))
Expect(r.maxReleaseHistory).To(PointTo(Equal(10)))
})
It("should allow setting the history to unlimited", func() {
Expect(WithMaxReleaseHistory(0)(r)).To(Succeed())
Expect(r.maxHistory).To(Equal(0))
Expect(r.maxReleaseHistory).To(PointTo(Equal(0)))
Copy link
Member

Choose a reason for hiding this comment

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

Is there some place that we can test that the default value is 10 when WithMaxReleaseHistory() is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked briefly and didnt see anywhere, ill take a closer look and write a new spec if i have to

})
It("should fail if value is less than 0", func() {
Expect(WithMaxReleaseHistory(-1)(r)).NotTo(Succeed())
Expand Down