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

Hkatz/vpa updates more tests and pointers #209

Merged

Conversation

hjkatz
Copy link
Contributor

@hjkatz hjkatz commented Aug 24, 2020

Why?

Here's the last PR from the set of changes Squarespace has made in their local fork. This last set of changes aims to improve the simplicity of the create/update/delete process for VPAs, Deployments, and Namespaces by using the real objects instead of string names throughout the reconciliation process. This comes with the added advantage of being able to test/mock these individual calls and methods.

What?

  • Use Namespace, VPA, and Deployment objects instead of string names
  • Always update VPAs during reconciliation for simplicity of the reconciler and ensuring the VPAs stay in sync
  • Use a helper func getVPAObject() to return an in-time state representation of the VPA and then use that for create/update/delete as needed
  • Add tests

Harrison Katz and others added 30 commits February 28, 2020 15:17
The VPAUpdateMode field will allow us to use Goldilocks to create VPAs
that operate outside of "off" mode.
This commit adds a parameter to createVPA that allows the user to
specify the update mode for the VPA. The mechanism for specifying this
is the label goldilocks.fairwinds.com/vpaUpdateMode.
Kubernetes labels are based on DNS names, which are case insensitive.
However, the current label used to specify the update mode of created
VPAs, vpaUpdateMode, is case sensitive. This has caused a bug that
results in all VPAs being created in "Off" mode. This commit fixes that
bug by using a case-insensitive label to specify the vpa-update-mode.
Also:
 - Update reconciler to use VPA and Deployment objects
   directly (instead of names)
 - Update reconciler logging
 - Update/Add vpa tests
Also:
 - Update vpa tests
 - Fix vpa-opt-out logic
Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com>
Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com>
Andrew Suderman and others added 20 commits August 5, 2020 11:01
Merge master from upstream
Co-authored-by: Andrew Suderman <andrew@sudermanjr.com>
…i-call

Hkatz/batch summary deployments api call
Hkatz/batch summary deployments api call (FairwindsOps#194)
Update master from upstream
@hjkatz
Copy link
Contributor Author

hjkatz commented Aug 24, 2020

Not sure how there are conflicts... everything is updated on my end. I'm going to fiddle with git and try to figure out what's going on and may close/reopen this PR.

@hjkatz
Copy link
Contributor Author

hjkatz commented Aug 24, 2020

Not sure how there are conflicts... everything is updated on my end. I'm going to fiddle with git and try to figure out what's going on and may close/reopen this PR.

Ok I figured out that I was missing the v1beta2 -> v1 changes so I've merged those.

@codecov
Copy link

codecov bot commented Aug 25, 2020

Codecov Report

Merging #209 into master will increase coverage by 1.29%.
The diff coverage is 88.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   51.57%   52.86%   +1.29%     
==========================================
  Files           8        8              
  Lines         605      611       +6     
==========================================
+ Hits          312      323      +11     
+ Misses        261      258       -3     
+ Partials       32       30       -2     
Impacted Files Coverage Δ
pkg/vpa/vpa.go 75.00% <88.13%> (+3.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 159ff4a...f27d0eb. Read the comment docs.

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.

Awesome! I have wanted to do that for a long time (RE: using vpa objects instead of names). Double awesome on the code coverage.

@sudermanjr sudermanjr merged commit 1845fa6 into FairwindsOps:master Aug 26, 2020
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