-
Notifications
You must be signed in to change notification settings - Fork 216
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
chore: Factor minValues requirements checking into the InstanceTypes methods #1246
chore: Factor minValues requirements checking into the InstanceTypes methods #1246
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonathan-innis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
86ae394
to
92f03ef
Compare
Pull Request Test Coverage Report for Build 9106406205Details
💛 - Coveralls |
@@ -136,8 +136,8 @@ func (m *MultiNodeConsolidation) firstNConsolidationOption(ctx context.Context, | |||
// required | |||
replacementHasValidInstanceTypes := false | |||
if cmd.Action() == ReplaceAction { | |||
cmd.replacements[0].InstanceTypeOptions = filterOutSameType(cmd.replacements[0], candidatesToConsolidate) | |||
replacementHasValidInstanceTypes = len(cmd.replacements[0].InstanceTypeOptions) > 0 | |||
cmd.replacements[0].InstanceTypeOptions, err = filterOutSameType(cmd.replacements[0], candidatesToConsolidate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we ever expect this error to be non-nil since min values is still already validated in compute consolidation? It doesn't hurt to have this check, but if there is a new non-nil scenario, I'm wondering if we should still silently fail here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could definitely be non-nil. If we filter out the same type and this breaks the minValues requirement, then we should fail. Also, conceptually, I'd like to keep the semantic with this to be: "when we filter we can fail if our constraints aren't maintained. An error indicates that our constraints weren't maintained."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, makes sense to me 👍 When I was reading the original implementation I missed that filterByPriceWithMinValues
would return an empty slice when violated so I thought this was a new error check.
92f03ef
to
4c2ce1d
Compare
4c2ce1d
to
5e7adf5
Compare
/lgtm |
Fixes #N/A
Description
This refactors the minValues code to be reusable by other places were instance types are used and truncated (like in aws/karpenter-provider-aws#6182). It also clarifies some of the usages and checking of minValues.
How was this change tested?
make presubmit
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.