Hkatz/add vpa update mode #137
Conversation
…airwindsOps#103)" This reverts commit e55026d.
Revamp e2e Tests (FairwindsOps#118)
Merge Master
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.
hjkatz
requested review from
lucasreed,
mgoodhart5 and
sudermanjr
as code owners
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
==========================================
- Coverage 44.65% 43.58% -1.07%
==========================================
Files 8 8
Lines 571 585 +14
==========================================
Hits 255 255
- Misses 304 317 +13
- Partials 12 13 +1
Continue to review full report at Codecov.
|
I'm fine with the copy paste method. We aren't going to cut a release until all of these are in most likely, so it will be thoroughly tested via e2e and manually by me before we release. Whatever is easiest for you is fine with me, so copy paste is good. |
There was a problem hiding this comment.
Overall looks great. Just a couple comments that I think should at least be discussed.
I never replied to your previous comment about updateMode, but we discussed it and we are okay with moving forward on an un-documented feature for it for now.
It may make sense for us to include more VPA management features in the future, but for now undocumented is fine.
|
I've updated the PR with 3 additional commits based on your feedback. Let me know what you think! 👍 |
hjkatz
force-pushed
the
hkatz/add-vpa-update-mode
branch
from
f46b0f0
to
bc50617
Compare
I noticed that the test cases we've added for the reconciler aren't part of these commits, but do trust that we have added them and they'll be coming in a future PR. We have tests for just about all of the reconciler's different behaviours :) |
|
Do y'all prefer that I click the "Resolve conversation" button when I submit additional commits to address comments? I've been using the checkmark/task thing in markdown
Either way, the additional commits have been added 👍 |
No preference. If they are resolved I just open them and review. |
As I'm creating these PRs and pulling the commits over one at a time into new PRs on this new fork, I'm starting to wonder if it might be better to hand craft these PRs or re-implement some of them by hand and submit net-new changes as PRs rather than try to copy/paste commit by commit from our internal fork to this public fork.
I don't mind doing the copy/paste method, but I'm worried that sometimes the PRs may include accidental changes or not quite as clean code as the final product due to future commits that haven't been pulled in yet. For example in this PR you can see that in the last PR (#128) we had forgotten to enable the dry run feature in the controller, but in a commit in this PR we added it back in.
The net result is the same, but the process we take to get there is different.
What are y'all's thoughts on this? Personally the copy/paste method is very straightforward and pretty easy to do for me, so I like that. I also want to be clear and transparent about the changes we're putting up and proposing to implement in your project.
Note: This comment gives context to this PR: #128 (comment)
Add a VPAUpdateMode to the vpa.Reconciler
The VPAUpdateMode field will allow us to use Goldilocks to create VPAs
that operate outside of "off" mode.
Add ability to specify vpa update 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.