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.

Sign up for GitHub

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

Jump to bottom

Update VPA k8s dependencies to 1.18.3 #3225

Merged
merged 6 commits into from
Jul 22, 2020
Merged

Update VPA k8s dependencies to 1.18.3 #3225

merged 6 commits into from
Jul 22, 2020

Conversation

bskiba
Copy link
Member

@bskiba bskiba commented Jun 15, 2020

Fixes #3219 #3097

Does similar work as #3098 but that PR has merge conflicts. I also updated the e2e test dependencies accordingly.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 15, 2020
@bskiba bskiba mentioned this pull request Jun 15, 2020
Closed
@bskiba bskiba changed the title Update VPA k8s dependencies to 1.18.3 [WIP] Update VPA k8s dependencies to 1.18.3 Jun 15, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2020
@bskiba bskiba force-pushed the vpa-to-k8s-1-18-3 branch 2 times, most recently from 03227c2 to 061276d Compare June 15, 2020 17:02
@bskiba
Copy link
Member Author

bskiba commented Jun 15, 2020

Unrelated test failing.

@bskiba bskiba changed the title [WIP] Update VPA k8s dependencies to 1.18.3 Update VPA k8s dependencies to 1.18.3 Jun 15, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2020
@bskiba
Copy link
Member Author

bskiba commented Jun 15, 2020

@krzysied would you be able to review?
These are all mechanical changes, no actual behavioral changes. I tried to split them into commits that make sense. Let me know if you would like me to split this in more PRs.

@bskiba
Copy link
Member Author

bskiba commented Jun 15, 2020

Actually, I would like to test this a little more, you can hold off the review for now.

@lilic lilic mentioned this pull request Jul 10, 2020
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2020
@bskiba bskiba force-pushed the vpa-to-k8s-1-18-3 branch from 061276d to 8ceb1a1 Compare July 21, 2020 12:14
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2020
@bskiba bskiba force-pushed the vpa-to-k8s-1-18-3 branch from 8ceb1a1 to 4f57dd6 Compare July 21, 2020 14:05
@bskiba bskiba force-pushed the vpa-to-k8s-1-18-3 branch from 4f57dd6 to 3edbe0c Compare July 21, 2020 14:32
@bskiba
Copy link
Member Author

bskiba commented Jul 21, 2020

Okay, this is ready for review
/assign @krzysied
Krzysztof, would you mind taking a look? As mentioned before, this only updates dependencies and generated code and adjusts code to work with new deps.

krzysied
krzysied reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@krzysied krzysied left a comment

Choose a reason for hiding this comment

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

Only nits.

vertical-pod-autoscaler/e2e/v1/autoscaling_utils.go
@@ -321,7 +316,7 @@ func (rc *ResourceConsumer) sendConsumeCustomMetric(delta int) {
framework.ExpectNoError(err)
}

// CleanUp func
// CleanUp clean up the background goroutines responsible for consuming resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/clean/cleans/

vertical-pod-autoscaler/e2e/v1/autoscaling_utils.go
)

var (
// KindRC var
// KindRC is the GVK for ReplicationController
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing dot at the end of the sentence. The same goes for other comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is mostly copied from k8s repo. I'm not entirely sure we need this copy, but I'd rather not mix it with this PR. The same goes for the other comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack.

@bskiba
Copy link
Member Author

bskiba commented Jul 21, 2020

/hold
until e2e tests are fixed

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2020
@krzysied
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krzysied

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2020
@tariq1890
Copy link
Contributor

Any updates on this?

@bskiba
Copy link
Member Author

bskiba commented Jul 22, 2020

This is on hold until e2e tests are fixed. #3340 which should fix them has been merged just now, so we should see green test runs today. Then this change will be merged.

@bskiba
Copy link
Member Author

bskiba commented Jul 22, 2020

Tests are green again \o/.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3850cc5 into kubernetes:master Jul 22, 2020
@mcristina422 mcristina422 mentioned this pull request Jul 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krzysied krzysied krzysied left review comments

@jbartosik jbartosik Awaiting requested review from jbartosik

Assignees

@krzysied krzysied

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

VerticalPodAutoscaler not compatible with k8s 1.18.x libraries
4 participants
@bskiba @krzysied @k8s-ci-robot @tariq1890