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

Added option to use VpaMinReplicasAnnotation from labels #720

Merged

Conversation

devantler
Copy link
Contributor

@devantler devantler commented Jul 21, 2024

This PR adds the option of using goldilocks.fairwinds.com/vpa-min-replicas: X as a label.

Checklist

  • I have signed the CLA
  • I have updated/added any relevant documentation

Description

Currently it is not possible to set vpa-min-relplicas with labels, like it is with some of the other options. This fix should ensure that is also the case for vpa-min-replicas

What's the goal of this PR?

To support labels for vpa-min-replicas

What changes did you make?

I added an if/else statement in the code where this was needed. The solution was inspired by how this is implemented other places in the code. Feel free to change it to your liking!

What alternative solution should we consider, if any?

blank

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@sudermanjr sudermanjr left a comment

Choose a reason for hiding this comment

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

I don't love only doing this for the minReplicas and not the other VPA settings. Can you make sure that all settings are updated?

@devantler
Copy link
Contributor Author

Sure, i'll have a look at it as soon as possible ☀️🕶️

@devantler
Copy link
Contributor Author

@sudermanjr I made a search for all cases that uses the accessor.GetAnnotations() method, and only found one more that was missing an else condition for retrieving the key/value from labels. The rest had both annotations and labels supported, where annotations is prioritized if both are set.

I believe that should be all, so I have rerequested a review ✌🏻

@sudermanjr
Copy link
Member

Looks good. Now I need to figure out why the CI won't run

@sudermanjr sudermanjr merged commit 2be84ac into FairwindsOps:master Jul 29, 2024
7 checks passed
@devantler devantler deleted the vpa-min-replicas-from-labels branch July 30, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants