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

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Add a default value for MaxReleaseHistory #291

merged 5 commits into from
Jul 8, 2024

Conversation

Jay-Madden
Copy link
Contributor

@Jay-Madden Jay-Madden commented Jan 30, 2024

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...

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2024
@joelanford
Copy link
Member

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.

@Jay-Madden
Copy link
Contributor Author

Jay-Madden commented Feb 13, 2024

@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 Jay-Madden marked this pull request as ready for review February 26, 2024 22:55
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2024
@Jay-Madden
Copy link
Contributor Author

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

})
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

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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     

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

@joelanford joelanford added this pull request to the merge queue Jul 8, 2024
Merged via the queue into operator-framework:main with commit 20a5c31 Jul 8, 2024
6 checks passed
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.

4 participants