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

change prune strategy to separate resource removal from the strategy … #76

Merged
merged 1 commit into from
Oct 30, 2021

Conversation

jmccormick2001
Copy link

Description of the change:
this change separates out the removal of resources logic from the various prune strategy implementations so that someone writing a custom strategy doesn't have to worry about implementing or calling resource removal logic themselves.

Motivation for the change:
this should make it easier for custom strategy implementations to be developed since they would not have to remember to call resource removal logic directly.

@openshift-ci
Copy link

openshift-ci bot commented Oct 28, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from jmccormick2001 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coveralls
Copy link

coveralls commented Oct 28, 2021

Pull Request Test Coverage Report for Build 1394911945

  • 29 of 43 (67.44%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 80.902%

Changes Missing Coverage Covered Lines Changed/Added Lines %
prune/maxage.go 5 7 71.43%
prune/maxcount.go 4 6 66.67%
prune/prune.go 7 9 77.78%
prune/remove.go 13 21 61.9%
Totals Coverage Status
Change from base Build 1392761236: -0.01%
Covered Lines: 610
Relevant Lines: 754

💛 - Coveralls

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

Ah nice. Basically you get the resources by a particular strategy then do the removal in one place. I like it.

@jmrodri
Copy link
Member

jmrodri commented Oct 29, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2021
Copy link

@ryantking ryantking left a comment

Choose a reason for hiding this comment

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

Looks like there is a bug with the max count strategy, details in comments on the file.

return err
}
default:
return fmt.Errorf("unsupported resource kind")
}

Choose a reason for hiding this comment

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

Can move each error check to a single one after the switch

prune/maxcount.go Show resolved Hide resolved
prune/maxcount.go Show resolved Hide resolved
@jmccormick2001 jmccormick2001 merged commit 10d4cb0 into operator-framework:main Oct 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants