helm-operator-plugins icon indicating copy to clipboard operation
helm-operator-plugins copied to clipboard

Add a default value for MaxReleaseHistory

Open Jay-Madden opened this issue 1 year ago • 4 comments

Currently the default for MaxReleaseHistory is 0 which translates to no maximum meaning all helm releases will be kept around indefinitely which is wasteful and unneeded as a default.

Open Question: What should the default be? 10 seems high to me...

Jay-Madden avatar Jan 30 '24 22:01 Jay-Madden

I think this is a good change. However, changing this default would be a breaking change for anyone that requires more than 10 releases in history, especially since we've explicitly documented via GoDoc that 0 is the default and that it means "unlimited".

I don't know if there are any such users, but I also don't know that there aren't.

Given that we are still pre-1.0, there is nothing stopping us from making this breaking change. But we should definitely remember to call it out specifically in the release notes and make sure the next tag is a minor version bump.

@Jay-Madden are you using helm-operator-plugins as a library? If so, you should be able to implement a separate default in your code in the meantime.

joelanford avatar Feb 13 '24 18:02 joelanford

@joelanford Yep we have already specified the max to 5 in our operator, it removed nearly 5000 secret objects lol.

This PR is just us trying to give back and set a sane default to prevent others from seeing the same problematic behavior.

Jay-Madden avatar Feb 13 '24 18:02 Jay-Madden

@joelanford @varshaprasad96 finally got around to updating the tests, should be good to go now

Jay-Madden avatar Feb 26 '24 22:02 Jay-Madden

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot avatar Apr 26 '24 16:04 openshift-merge-robot

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 48.73%. Comparing base (08ab7fb) to head (4295f8e). Report is 22 commits behind head on main.

:exclamation: There is a different number of reports uploaded between BASE (08ab7fb) and HEAD (4295f8e). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (08ab7fb) HEAD (4295f8e)
2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #291       +/-   ##
===========================================
- Coverage   85.06%   48.73%   -36.34%     
===========================================
  Files          19       62       +43     
  Lines        1346     2916     +1570     
===========================================
+ Hits         1145     1421      +276     
- Misses        125     1415     +1290     
- Partials       76       80        +4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 08 '24 13:07 codecov-commenter